-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: Total calculation in stacked Timeseries charts #24477
fix: Total calculation in stacked Timeseries charts #24477
Conversation
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx
Outdated
Show resolved
Hide resolved
db20f0c
to
07cca12
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.
Thanks for the fix + cleanup. One concern about bloating ChartProps
, but other than that LGTM. Let's try to find some time to hack on Plugin Arch v2 sometime!
/** Legend state */ | ||
legendState?: LegendState; |
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 want to raise the concern of bloating the number of different states we have here: Would it make sense to introduce something more generic, like chartState
that could be leveraged for transporting other info, too? I assume this isn't the last time we need to ship back info from the plugin to transformProps
, and having to add a new property can become tedious at some point. Thoughts? Alternatively, let's not do this now, but do a full cleanup when we get around to building the new React-based plugin arch (v2).
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 totally agree with your concern. I have many other concerns regarding the current plugin architecture but I think the best approach would be to stop amending the flawed design and think about a cleaner v2. One thing I think is really hard with the current structure is to properly test the plugin behaviors.
(cherry picked from commit c5b4ecd)
SUMMARY
This PR fixes the following bugs:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2023-06-21.at.19.07.26.mov
Screen.Recording.2023-06-21.at.18.56.50.mov
TESTING INSTRUCTIONS
1 - Make sure the totals are correctly calculated for Timeseries charts when interacting with the legend.
2 - Make sure that double clicking on a series exclusively selects it
ADDITIONAL INFORMATION