-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: add Nightingale chart support for echarts pie chart #28597
Conversation
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.
Pretty cool! Are you aware of any feature-parity issues that would prevent a migration from the legacy rose chart to this? I'm always excited to deprecate/remove a legacy plugin in favor of a newer option :) Also, would you mind adding this to the React Storybook entry? We're making a renewed effort to capture as many variants as possible there (interactive or static). |
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.
Thank you for the PR @hexcafe. Could you also add a screenshot of a Nightingale chart to the Pie viz gallery examples and make sure the Pie chart appears if you search for Nightingale? This will allow people to discover the chart.
Right now we have 4 screenshots. I think you can remove the last 2 and add yours.
superset-frontend/plugins/plugin-chart-echarts/src/Pie/controlPanel.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Pie/types.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Pie/types.ts
Outdated
Show resolved
Hide resolved
@rusackas I checked and the legacy plugin has no additional feature that would prevent us from migrating.
|
@michael-s-molina Thanks! I have updated the metadata. Now, we can discover the chart by searching for "Nightingale". |
@rusackas @michael-s-molina Should we upgrade to ECharts v5.5.0 soon? please take a look at #27341 , Thanks! |
Hi @michael-s-molina , Sorry for the delay. I have fixed the type errors. I hope this PR can be merged soon. Thank you! |
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.
@hexcafe Thank you for the feature and for addressing all comments!
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.
LGTM2! Thank you for this! Approving CI to run, and will spin up a test environment just to give it one last test drive :)
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.216.33.80:8080. Credentials are |
Works great! Thanks again! |
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit f9d2451)
SUMMARY
Add Nightingale Chart Support for Echarts Pie Chart
Echarts now supports Nightingale charts in pie charts, with two optional modes:
radius
: Uses the central angle to show the percentage of data and the radius to show data size.area
: All sectors share the same central angle, with data size shown only through radiuses.I have added a new option, 'ROSE TYPE,' to support this feature.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION