fix(plugin-chart-echarts): sort tooltip correctly #30819
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
viz:charts:echarts
villebro
commented
Nov 1, 2024
| ): string[] { | ||
| if (richTooltip && tooltipSortByMetric) { | ||
| return forecastValue | ||
| .slice() |
There was a problem hiding this comment.
Slicing to not mutate the input value (this would have caused issues in the new tests)
villebro
commented
Nov 1, 2024
Comment on lines
-254
to
+255
| Object.values(rawSeries).map(series => series.name as string), | ||
| rawSeries.map(series => series.name as string), |
There was a problem hiding this comment.
Bycatch: I'm not sure why we were doing Object.values on an array, maybe a typo
eschutho
approved these changes
Nov 1, 2024
sadpandajoe
added
the
v4.1
sadpandajoe
pushed a commit
that referenced
this pull request
Nov 1, 2024
(cherry picked from commit b02d18a)
🤣 🤣 🤣 🤣 🤣 🤣 |
github-actions
bot
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
The main ECharts series chart tooltips aren't sorting correctly. This is due to the tooltip keys being populated from
Object.keysfrom an object that was originally an array, which of course doesn't have a guaranteed sort order. This issue appears to have been there for a very long time: I treid to hunt it down with Blame, and I stopped searching when I got to three years ago where the code had already changed considerably, but the same bug seemed to be there.This PR fixes the bug, DRYs the code as the same logic is also needed in Mixed Timeseries chart, and adds simple tests for the new util. Naturally a full
transformPropsintegration test would be preferable here, but it felt like too much added code for such a small fix. Also, the util test looks concise enough for people to understand what it's used for.P.S. In the screenshots below please excuse my awful theme colors (I have those in my devenv to remind myself of the importance of theming work)
P.P.S. I suggest reviewing with hidden whitespace changes, as the changes are mostly that.
AFTER
Notice how "Tooltip sort by metric" is working as expected (biggest metric values are on the bottom due to stacking):
Also sorting series by "Category name" works as expected (this can be seen comparing the tooltip colors vs the bars):
BEFORE
Currently "Tooltip sort by metric" doesn't do anything (metric values are not monotonically growing):
When not sorting by metric, the series sort order isn't reflected in the tooltip order (the order is not the same in tooltip as on the bars):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION