Repository navigation
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| <LegendViewport data-test="timeseries-custom-legend" maxHeight={maxHeight}> | ||
| <LegendViewport | ||
| data-test="timeseries-custom-legend" | ||
| maxHeight={isVertical ? 0 : maxHeight} |
There was a problem hiding this comment.
Suggestion: For left and right legends this discards the computed chart-body height, so long lists have no height limit and grow past the chart instead of scrolling.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Css layout issue
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/TimeseriesLegend.tsx
**Line:** 175:175
**Comment:**
*Css Layout Issue: For left and right legends this discards the computed chart-body height, so long lists have no height limit and grow past the chart instead of scrolling.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 418deb6.
LegendViewport now receives maxHeight for both vertical and horizontal legends, preserving the computed height limit for left and right legends.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of fa8470e.
LegendViewport now receives maxHeight for both vertical and horizontal legends, preserving the computed height limit for left and right legends.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of d065af1.
LegendViewport now receives maxHeight for vertical as well as horizontal legends, retaining the computed height limit for left and right legends.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 45bfce3.
LegendViewport now receives maxHeight for vertical as well as horizontal legends, retaining the computed height limit for left and right legends.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 51f5da8.
LegendViewport now receives maxHeight for vertical as well as horizontal legends, retaining the computed height limit for left and right legends.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 2b0f86f.
LegendViewport now receives maxHeight for every orientation, including left and right legends; only maxWidth remains conditional on vertical orientation.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 9e1f43b.
LegendViewport now receives maxHeight for every orientation, including left and right legends; only maxWidth remains conditional on vertical orientation.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 910da1d.
LegendViewport now receives maxHeight for every orientation, including left and right legends; only maxWidth remains conditional on vertical orientation.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 4d18868.
LegendViewport now receives maxHeight for both vertical and horizontal legends; only maxWidth remains conditional on vertical orientation.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Reusing the HTML legend from #43314 is the right way to get Left/Right out of the canvas overlap, and the approach holds up.
Two things to sort before this goes in: the legend always takes the full 40% of the width even when the labels are short, and the left/right transformProps tests still assert the old native path. Notes inline. The tsc failure in lint-frontend (TimeGranularity in prophetOperator.ts) looks unrelated to this diff, a rebase on master should clear it.
| overflow-y: auto; | ||
| padding: ${theme.sizeUnit}px ${theme.sizeUnit * 2}px; | ||
| width: 100%; | ||
| width: ${maxWidth !== undefined && maxWidth > 0 ? `${maxWidth}px` : '100%'}; |
There was a problem hiding this comment.
This pins the legend to exactly 40% of the chart width for Left/Right, so a chart with two short series like "A" and "B" still gives up 40% of its plot to a mostly empty column. The native legend sized to its content, so simple charts get visibly worse.
max-width on line 30 already does the capping. With auto here the column should size to the longest label and stop at the cap:
| width: ${maxWidth !== undefined && maxWidth > 0 ? `${maxWidth}px` : '100%'}; | |
| width: ${maxWidth !== undefined && maxWidth > 0 ? 'auto' : '100%'}; |
| const isVertical = | ||
| orientation === 'left' || orientation === 'right'; |
There was a problem hiding this comment.
oxfmt rewrites this and EchartsTimeseries.tsx:839, which is what's failing pre-commit.
Running pre-commit run on the branch fixes both:
| const isVertical = | |
| orientation === 'left' || orientation === 'right'; | |
| const isVertical = orientation === 'left' || orientation === 'right'; |
| selected={item.selected} | ||
| /> | ||
| <Label>{item.name}</Label> | ||
| <Label truncate={isVertical}>{item.name}</Label> |
There was a problem hiding this comment.
Once a label gets cut to an ellipsis there's no way to read the full series name, and long groupby names are exactly the case this PR is fixing.
A title gives it back on hover:
| <Label truncate={isVertical}>{item.name}</Label> | |
| <Label | |
| title={isVertical ? item.name : undefined} | |
| truncate={isVertical} | |
| > | |
| {item.name} | |
| </Label> |
| legendOrientation === LegendOrientation.Bottom); | ||
| legendOrientation === LegendOrientation.Bottom || | ||
| legendOrientation === LegendOrientation.Left || | ||
| legendOrientation === LegendOrientation.Right); |
There was a problem hiding this comment.
keeps plain/left legends on the native ECharts path and its /right twin in test/Timeseries/transformProps.test.ts:1280 still assert the old behavior, so they're what's red in sharded-jest-tests (2).
Could you flip them to expect the custom legend, and add coverage for the new pieces too? getTimeseriesLegendMaxWidth and a TimeseriesLegend render with orientation: 'right' (column layout, truncation) would do it. Nothing on the vertical path is tested yet.
fa8470e to
d065af1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45520 +/- ##
=======================================
Coverage 84.78% 84.78%
=======================================
Files 3074 3074
Lines 206344 206371 +27
Branches 48601 48624 +23
=======================================
+ Hits 174946 174977 +31
+ Misses 28100 28097 -3
+ Partials 3298 3297 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
45bfce3 to
3eabe17
Compare
3eabe17 to
51f5da8
Compare
2b0f86f to
9e1f43b
Compare
There was a problem hiding this comment.
The Left/Right column now sizes to its content and stops at the 40% cap, truncated labels show the full name on hover, and the old native-path tests are flipped to the custom legend. The new tests cover the column layout, the dropped row-break sentinels, the zero-width cutoff and getTimeseriesLegendMaxWidth. I also checked that the grid stops reserving room for the hidden native legend, and that the ellipsis works inside the inline-flex button. CI is green, so nothing more from me.
…event overflow Extends the HTML TimeseriesLegend (introduced in apache#43314 for Top/Bottom) to also cover Left and Right orientations when legend type is Plain (List). Previously, Left and Right List legends used the ECharts canvas legend, which reserved space from a static width estimate based on the longest label. When many series were present the legend wrapped into multiple columns that drew over the plot and y-axis labels. Changes: - types.ts: extend TimeseriesCustomLegend.orientation from Top|Bottom to all four LegendOrientation values - transformProps.ts: expand usesCustomLegend condition to include Left and Right orientations with Plain type; update buildTimeseriesCustomLegend signature accordingly - TimeseriesLegend.tsx: add vertical layout support (column flex-direction, label truncation with ellipsis, maxWidth prop) for Left/Right legends; maxHeight behavior for Top/Bottom unchanged - EchartsTimeseries.tsx: add getTimeseriesLegendMaxWidth (capped at 40% of chart width), compute maxWidth for Left/Right legends, pass it to TimeseriesLegend; shouldRenderCustomLegend uses the correct dimension check per orientation Fixes apache#45490
Previously maxHeight was set to 0 for vertical (Left/Right) legends in LegendViewport, which produced 'max-height: none' in CSS — letting long legend lists grow past the chart boundary without scrolling. EchartsTimeseries already computes customLegendMaxHeight as chartBodyHeight for vertical legends. Pass that value through to LegendViewport so the legend is capped and scrollable. Also strengthens the early-return guard: both maxHeight and maxWidth (for vertical legends) are checked so the component does not render when either dimension is zero.
…no-duplicates error The LegendOrientation import from '../types' was added as a separate import statement while '../types' was already imported on line 42. Merge both into one import to satisfy oxlint's no-duplicates rule.
…TimeseriesLegend component
…dge cases Widen buildTimeseriesCustomLegend's orientation parameter to LegendOrientation so the Left/Right path type-checks, skip ECharts row-break sentinels in the vertical layout where flex-basis maps to height, and apply the compact-chart height guard only to horizontal legends, which are the ones that compete with the plot for vertical space. Tests now assert the legend bounds, column layout, label truncation and row-break handling they claim to cover.
9e1f43b to
910da1d
Compare
| }); | ||
|
|
||
| const makeVerticalLegend = ( | ||
| orientation: LegendOrientation.Left | LegendOrientation.Right, |
There was a problem hiding this comment.
This is the tsc failure in lint-frontend (TS2345 at line 1588). The test.each([LegendOrientation.Left, LegendOrientation.Right]) below infers plain LegendOrientation, which doesn't fit this narrowed parameter.
The object gets cast to never anyway, so widening the parameter is the smallest fix:
| orientation: LegendOrientation.Left | LegendOrientation.Right, | |
| orientation: LegendOrientation, |
github-actions
Bot
added
the
requires:rebase
# Conflicts: # superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/TimeseriesLegend.tsx
github-actions
Bot
removed
the
requires:rebase
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Follow-up to my earlier pass at 910da1d (#45520 (review)), now on the merge commit.
The only new commit is the upstream merge. It widens the makeVerticalLegend parameter, which clears the tsc failure I flagged, and picks up the icon field that #45648 made required on legend items. TimeseriesLegend.tsx merged cleanly, the delta there is exactly #45648's hunks and nothing from this PR moved. lint-frontend and all eight jest shards are green and the branch is mergeable.
Nothing new to add inline. Still LGTM.
SUMMARY
Extends the HTML-based
TimeseriesLegend, introduced in #43314 for Top/Bottom orientations, to support Left and Right orientations when the legend type is List (Plain).Previously, Left and Right List legends used the ECharts canvas legend, which could wrap into multiple columns and overlap the chart plot and y-axis labels when many series were present.
What changed:
usesCustomLegendcondition intransformProps.tsto include Left and Right orientations for Plain legends.TimeseriesCustomLegendto support all fourLegendOrientationvalues.maxWidthprop toTimeseriesLegendand implemented a vertical layout with label truncation for Left/Right legends.getTimeseriesLegendMaxWidth()inEchartsTimeseriesto cap the width of Left/Right legends at 40% of the chart width.Fixes #45490
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Left/Right List legends could wrap into multiple columns and overlap the chart plot and y-axis labels.
After: The custom HTML legend renders in a bounded vertical column beside the plot, with truncated labels and scrolling.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION