fix(echarts): use scroll legend for horizontal layouts to prevent overlap #36306
Conversation
Code Review Agent Run #b919b0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Can we get before/after screenshots or videos? |
| orientation: LegendOrientation, | ||
| show: boolean, | ||
| theme: SupersetTheme, | ||
| theme?: SupersetTheme, |
There was a problem hiding this comment.
Any reason to make this optional? Just curious
There was a problem hiding this comment.
There’s no strong reason for making theme optional.
That came from an earlier experiment and I forgot to change it back.
I’ll keep theme required and continue using it for the legend/selector styling so existing behaviour isn’t changed.
| }, | ||
| type: effectiveType, | ||
| }; | ||
| const MIN_LEGEND_WIDTH = 0; |
There was a problem hiding this comment.
Are we sure we want to remove all these settings? Looks dangerous.
There was a problem hiding this comment.
You’re right, this removed more than necessary.
I’ll restore the existing legend settings (selector buttons, theming, padding/text handling, etc.) and limit the change to just adjusting the legend type for horizontal layouts so we can use scroll without breaking current behaviour.
| legend: expect.objectContaining({ | ||
| show: true, | ||
| type: 'scroll', | ||
| selector: ['all', 'inverse'], |
There was a problem hiding this comment.
I don't think we want to remove these buttons. They're quite helpful.
There was a problem hiding this comment.
@YousufFFFF I think we want these buttons back. They're quite handy :D
There was a problem hiding this comment.
Yess sure, I'll add them back in the transformProps.test.ts!
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Looks like there's a lot of stuff being removed/disabled here that would be considered breaking changes.
There was a problem hiding this comment.
Pull request overview
This PR aims to fix legend overlap issues in ECharts by automatically upgrading plain legends to scrollable legends for horizontal orientations (top/bottom). However, the implementation has several critical bugs that break existing functionality.
Key Changes:
- Automatic conversion of plain legend type to scroll type for horizontal orientations (top/bottom)
- Significant simplification of the
getLegendPropsfunction - Removal of theme-related legend properties and padding-based adjustments
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts |
Modified getLegendProps to auto-convert plain legends to scroll for horizontal layouts; removed theme properties, legend state, and padding logic |
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts |
Updated tests to reflect new scroll behavior; removed theme property validations |
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts |
Removed selector field assertion from legend test to match implementation changes |
| theme: SupersetTheme, | ||
| theme?: SupersetTheme, | ||
| zoomable = false, | ||
| legendState?: LegendState, |
There was a problem hiding this comment.
The legendState parameter is accepted but never used in the function body. This breaks legend state persistence - when users toggle legend items on/off, their selections won't be preserved. The selected property should be set in the legend object:
const legend: LegendComponentOption | LegendComponentOption[] = {
orient: isHorizontal ? 'horizontal' : 'vertical',
show,
type: effectiveType,
selected: legendState,
};| theme?: SupersetTheme, | ||
| zoomable = false, | ||
| legendState?: LegendState, | ||
| padding?: LegendPaddingType, |
There was a problem hiding this comment.
The padding parameter is accepted but never used in the function body. This removes important functionality where legend text was truncated based on available space for left/right orientations, and legend positioning was adjusted based on padding for top/bottom orientations.
For left/right orientations, the original implementation set:
legend.textStyle = {
overflow: 'truncate',
width: getLegendWidth(padding.left), // or padding.right
};For top/bottom orientations, it set:
legend.left = padding.left;Either this parameter should be removed from the function signature, or the padding logic should be restored.
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
Outdated
Show resolved
Hide resolved
| borderColor: theme.colorBorder, | ||
| }, | ||
| }; | ||
| const expectedThemeProps = {}; |
There was a problem hiding this comment.
The expectedThemeProps object was changed from containing theme-related legend properties to an empty object. This means the tests are no longer validating that theme properties (selector, selectorLabel) are correctly applied to legends.
If these properties are intentionally removed from the implementation (as seen in series.ts), the tests should be updated to explicitly verify they are NOT present, rather than spreading an empty object. If they should be present, the tests need to be reverted to validate them properly.
There was a problem hiding this comment.
Since I removed the selector: ['all', 'inverse'] from the transformProps.test.ts, It was necessary to remove these from it too ( To pass the sanity checks)
I'll restore everything quickly!
| const effectiveType = | ||
| type === LegendType.Scroll || !isHorizontal ? type : LegendType.Scroll; | ||
|
|
||
| const legend: LegendComponentOption | LegendComponentOption[] = { |
There was a problem hiding this comment.
[nitpick] The legend variable is declared with type LegendComponentOption | LegendComponentOption[], but it's always initialized as a single object and never as an array. This requires unnecessary type casting throughout the switch statement.
Consider either:
- Changing the type to just
LegendComponentOptionif arrays are not needed - Or keeping the union type but initializing
legendwith explicit type assertion:const legend = { ... } as LegendComponentOption;
This would eliminate the need for repeated type casting on lines 464, 469, 473, and 474.
There was a problem hiding this comment.
@YousufFFFF is there ever an array here? If not, simplifying is good :)
There was a problem hiding this comment.
There is never an array being used for legend, so I will simplify it to just:
const legend: LegendComponentOption = { ... }
This removes the unnecessary union type and casting throughout the switch cases.
| case LegendOrientation.Top: | ||
| legend.top = 0; | ||
| legend.right = zoomable ? TIMESERIES_CONSTANTS.legendTopRightOffset : 0; | ||
| if (padding?.left) { | ||
| legend.left = padding.left; | ||
| } | ||
| break; | ||
| default: |
There was a problem hiding this comment.
[nitpick] The LegendOrientation.Top case falls through to the default case. While this works correctly, it would be clearer and more maintainable to either:
- Remove the explicit
case LegendOrientation.Top:line and just handle it in the default - Or add a comment explaining the intentional fall-through
- Or combine them:
case LegendOrientation.Top: default:
This makes the intent clearer that Top and the default case are handled identically.
Sure! |
Thanks a lot for the detailed review, this is very helpful. |
…ng existing legend features
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
YousufFFFF
force-pushed
the
fix/echarts-legend-scroll
branch
from
00934a9 to
4d34219
Compare
|
Update:
Before (current behavior):
After (with this PR):
This change only affects the legend configuration and does not modify any other behavior. |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Most of this is looking good... thanks for the updates/screenshots. I left a few comments/questions, but the biggest concern is about the removal of the All/Inv buttons, which we still want.
|
Thanks for the review, @rusackas! 1.)Just to clarify the concern about the All/Inv buttons, they were not removed and still work as expected with the scroll legend. I’ve added a short before/after video in the comment showing:
The scroll mode only changes the legend layout (to prevent overflow), but does not modify the built-in ECharts legend controls. 2.)Regarding the LegendComponentOption | LegendComponentOption[] to just: LegendComponentOption Please let me know if you'd like the video embedded directly in the pr summary or if any further adjustments are needed! DEV.-.Google.Chrome.2025-12-03.02-39-57.mp4 |
|
CodeAnt AI is reviewing your PR. |
codeant-ai-for-open-source
bot
added
the
size:M
|
CodeAnt AI finished reviewing your PR. |
|
Hi @rusackas, I’ve pushed the final updates: • Applied the Please let me know if you'd like any further adjustments! |
|
Heya... just a few outstanding questions:
Otherwise, I think we're about there... the basic fix looks good, just trying to make sense of the other changes and if/why they're necessary :D Thanks again for seeing this through! |
|
Thanks a lot for the thoughtful review, @rusackas really appreciate the clarity on these points! You're absolutely right about the I’ll revert that change so the test continues validating the full legend object, as before. Thanks for calling that out. I definitely don’t want to weaken the test coverage. Regarding the All/Inv buttons: yes, they are still present in the actual output (as expected), so removing them from the tests was unintentional on my part. I’ll restore those assertions so the test properly reflects the real behavior. I’ll push the corrective updates shortly and re-run the test suite to confirm everything aligns again. Thanks again for the thorough guidance and patience, really helping me understand the expectations and avoid regression🙏 |
|
CodeAnt AI is running Incremental review |
|
Hey @rusackas!, I’ve pushed an update: Also, the pre-commit CI seems to have failed on an unrelated step (helm-docs broken pipe). |
2 participants
User description
fix(echarts): enable scroll legend for horizontal layouts to prevent overlap
SUMMARY
Charts with many legend items can overlap or obscure the chart area when the legend is placed at the top or bottom. ECharts supports a scrollable legend, which prevents this issue by allowing legend items to scroll instead of expanding into the chart.
This PR introduces a sensible default:
TopandBottom), if the user-selected legend type isPlain, it is automatically upgraded toScroll.This prevents legend overlap without requiring additional configuration from users.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
AFTER:
DEV.-.Google.Chrome.2025-12-02.16-09-14.mp4
TESTING INSTRUCTIONS
Type checking and frontend linting pass locally, and CI runs full Jest suite.
ADDITIONAL INFORMATION
CodeAnt-AI Description
Make horizontal chart legends scrollable to avoid overlap
What Changed
Impact
✅ No legend overlap when many series are shown✅ Clearer charts with long legends✅ Less manual tweaking of legend settings💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.