Skip to content

feat: bumping echarts plugin, adding new treemap plugin#14560

Merged
zhaoyongjie merged 4 commits into
masterfrom
bumping-echarts
May 13, 2021
Merged

feat: bumping echarts plugin, adding new treemap plugin#14560
zhaoyongjie merged 4 commits into
masterfrom
bumping-echarts

Conversation

@rusackas
Copy link
Copy Markdown
Member

@rusackas rusackas commented May 10, 2021

SUMMARY

Bumps the Echarts plugin, and replaces the old treemap with the new one. We need to consider a proper migration, but this PR and its ephemeral environment can serve as a preview for both the new treemap, and for the updated/polished funnel.

@junlincc

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:
image

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas rusackas marked this pull request as draft May 10, 2021 23:18
@rusackas rusackas added the request:qa-review Requests a QA review label May 10, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2021

Codecov Report

Merging #14560 (8b156c4) into master (3f6bd1e) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14560      +/-   ##
==========================================
+ Coverage   77.38%   77.47%   +0.08%     
==========================================
  Files         959      959              
  Lines       48465    48465              
  Branches     5678     5678              
==========================================
+ Hits        37506    37547      +41     
+ Misses      10759    10718      -41     
  Partials      200      200              
Flag Coverage Δ
hive 80.96% <ø> (ø)
javascript 72.50% <ø> (ø)
mysql 81.24% <ø> (+<0.01%) ⬆️
postgres 81.26% <ø> (ø)
presto 80.96% <ø> (?)
python 81.79% <ø> (+0.15%) ⬆️
sqlite 80.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-frontend/src/visualizations/presets/MainPreset.js 0.00% <ø> (ø)
superset/models/core.py 89.13% <0.00%> (+0.27%) ⬆️
superset/connectors/sqla/models.py 90.26% <0.00%> (+1.44%) ⬆️
superset/db_engine_specs/presto.py 90.31% <0.00%> (+5.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f6bd1e...8b156c4. Read the comment docs.

@rusackas
Copy link
Copy Markdown
Member Author

/testenv up

@github-actions
Copy link
Copy Markdown
Contributor

@rusackas Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Copy Markdown
Contributor

@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.

@junlincc junlincc added the hold:testing! On hold for testing label May 10, 2021
@junlincc
Copy link
Copy Markdown
Member

because we didn't give the new chart a new name, looks like all the old treemap already migrated to echarts treemap? that will be great we dont even need migration script. 😅

new TableChartPlugin().configure({ key: 'table' }),
new TimePivotChartPlugin().configure({ key: 'time_pivot' }),
new TimeTableChartPlugin().configure({ key: 'time_table' }),
new TreemapChartPlugin().configure({ key: 'treemap' }),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not a direct replacement from nvd3-treemap to echart -treemap

  1. We should need some migration scripts, adapt to the new Treemap.
  2. fix original cypress CI

@junlincc
Copy link
Copy Markdown
Member

@rusackas can you push a commit to rename the echarts treemap to treemap 2 to get the ci pass? my understanding is that we still need to work on the migration script....

@zhaoyongjie zhaoyongjie marked this pull request as ready for review May 12, 2021 16:43
@zhaoyongjie
Copy link
Copy Markdown
Member

zhaoyongjie commented May 12, 2021

thanks @stephenLYZ for your hard works!

image

cc: @junlincc @rusackas

@zhaoyongjie zhaoyongjie changed the title feat: [WIP] bumping echarts plugin, adding new treemap plugin feat: bumping echarts plugin, adding new treemap plugin May 12, 2021
@junlincc junlincc removed the hold:testing! On hold for testing label May 13, 2021
@junlincc
Copy link
Copy Markdown
Member

/testenv up

@junlincc junlincc removed the request:qa-review Requests a QA review label May 13, 2021
@junlincc
Copy link
Copy Markdown
Member

@rusackas @zhaoyongjie can we get the checks pass? thank you for bumping the plugins!

@junlincc junlincc added the need:review Requires a code review label May 13, 2021
@github-actions
Copy link
Copy Markdown
Contributor

@junlincc Ephemeral environment spinning up at http://34.221.64.152:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

"@superset-ui/legacy-preset-chart-deckgl": "^0.4.6",
"@superset-ui/legacy-preset-chart-nvd3": "^0.17.42",
"@superset-ui/plugin-chart-echarts": "^0.17.44",
"@superset-ui/plugin-chart-echarts": "^0.17.47",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think bumping this to 0.17.47 without also bumping superset-ui/core first to 0.17.46 might be problematic. I suggest merging this first: #14547 , after which this should update cleanly.

@junlincc junlincc added the hold! On hold label May 13, 2021
Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@zhaoyongjie zhaoyongjie merged commit 568061e into master May 13, 2021
@github-actions
Copy link
Copy Markdown
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas rusackas deleted the bumping-echarts branch May 14, 2021 00:20
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* chore: bumping echarts plugin

* feat: Upgrading to new treemap

* bump @superset-ui/plugin-chart-echarts 0.17.47

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* chore: bumping echarts plugin

* feat: Upgrading to new treemap

* bump @superset-ui/plugin-chart-echarts 0.17.47

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* chore: bumping echarts plugin

* feat: Upgrading to new treemap

* bump @superset-ui/plugin-chart-echarts 0.17.47

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
* chore: bumping echarts plugin

* feat: Upgrading to new treemap

* bump @superset-ui/plugin-chart-echarts 0.17.47

Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels hold! On hold need:review Requires a code review preset-io size/XS 🚢 1.3.0 First shipped in 1.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants