-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(plugin-chart-echarts): supports sunburst chart v2 [WIP] #21625
feat(plugin-chart-echarts): supports sunburst chart v2 [WIP] #21625
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21625 +/- ##
==========================================
- Coverage 67.13% 67.00% -0.13%
==========================================
Files 1869 1876 +7
Lines 71656 71812 +156
Branches 7822 7863 +41
==========================================
+ Hits 48103 48115 +12
- Misses 21526 21671 +145
+ Partials 2027 2026 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool work @stephenLYZ! I left some first-pass comments.
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
Outdated
Show resolved
Hide resolved
/testenv up |
@stephenLYZ Ephemeral environment spinning up at http://54.149.133.75:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @stephenLYZ ! 🚀 One minor comment about avoiding using deprecated controls. I also agree with @michael-s-molina's comment about using the theme object here wherever possible so we don't introduce more styling debt.
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx
Outdated
Show resolved
Hide resolved
@stephenLYZ one more thing - could we add at least one simple storybook? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenLYZ Thanks for taking the new Sunburst chart! I have tested in my local. I was doing the first-round review.
- When the secondary metric is used, the color of the dimensions seems broken.
-
the description of the Hierarchy should add some explanation that the first level of the hierarchy is the innermost circle.
-
Should use a query to get the Total value of the metric instead of the frontend calculating it? because there are some reasons(Javascript with int8/int16?) that caused incorrect total values.
Screen.Recording.2022-10-24.at.17.30.29.mov
7e90904
to
69c26ab
Compare
69c26ab
to
6956cc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted a few commits to be able to merge this PR. I unregistered the chart from the available visualizations and will tackle the following open issues in another PR:
- Add thumbnail and example pictures
- Add a Storybook entry for the chart
- When the secondary metric is used, the color of the dimensions seems broken.
- Make the relationship between hierarchies and the circles more clear to the user
- Use the callback function and return an HTML element for the tooltip
- Improve the tooltip description to show the hierarchy names
- Improve tooltip positioning as it is sometimes being cut off the screen
- Fix the calculation of total values
- Register the chart again so it's available for use
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
This PR supports sunburst chart v2 which is powered by echart.
Features:
Todo:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
base
show label
show total
tooltip
secondary metric
cross-filter
2022-09-29.12.13.56.mov
drill-to-detail
2022-09-29.12.14.36.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION