-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
fix(plugin-chart-echarts): reorder totals and support multimetric sort #23675
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23675 +/- ##
==========================================
- Coverage 66.40% 66.39% -0.01%
==========================================
Files 1922 1922
Lines 74042 74048 +6
Branches 8102 8104 +2
==========================================
+ Hits 49166 49167 +1
- Misses 22805 22809 +4
- Partials 2071 2072 +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 |
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx
Outdated
Show resolved
Hide resolved
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.
LGTM with tiny naming nit
cc3f855
to
6b01df3
Compare
SUMMARY
When using the multiseries sort on generic x-axis, the totals are not sorted, causing them to display on the wrong bars. In addition, it was not possible to sort by the total bar height if there were multiple metrics. This PR fixes both issues.
AFTER
When there are two metrics, the multiseries sort control is now displayed. In addition, the totals are displayed on the correct rows:
total-after.mp4
BEFORE
Previously, it wasn't possible to sort by the total if there were multiple metrics (notice that one would commonly want to display NY before TX and PA before OH):
In addition, when using the multiseries sort control, the totals would be displayed on the original bars, as they didn't get reordered along with the chart data (notice that the last bar shows 37.1 M births, despite the total of the series being 5.62M):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION