Repository navigation
fix(explore): honor explicit List legend selection instead of flipping to scroll #41749
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
Changes from all commits
dfcea12
e0ce25a
849cde5
dfbf860
37fc0a4
af6e282
60379f6
8bc109f
f5c62a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,11 +60,9 @@ const DEFAULT_LEGEND_ICON_WIDTH = 25; | |
| const LEGEND_ICON_LABEL_GAP = 5; | ||
| const LEGEND_HORIZONTAL_SIDE_GUTTER = 16; | ||
| const LEGEND_HORIZONTAL_ROW_HEIGHT = 24; | ||
| const LEGEND_HORIZONTAL_MAX_ROWS = 2; | ||
| const LEGEND_HORIZONTAL_MAX_HEIGHT_RATIO = 0.25; | ||
| // Cap the reserved horizontal legend margin so an overflowing legend can't eat the plot. | ||
| const MAX_LEGEND_MARGIN_RATIO = 0.4; | ||
| const LEGEND_VERTICAL_SIDE_GUTTER = 16; | ||
| const LEGEND_VERTICAL_ROW_HEIGHT = 24; | ||
| const LEGEND_VERTICAL_MAX_WIDTH_RATIO = 0.4; | ||
| const LEGEND_SELECTOR_GAP = 10; | ||
| const LEGEND_MARGIN_GUTTER = 45; | ||
| // ECharts does not expose pre-render measurements for plain legends, so these | ||
|
|
@@ -84,10 +82,6 @@ export type LegendLayoutResult = { | |
| effectiveType: LegendType; | ||
| }; | ||
|
|
||
| const SCROLL_LEGEND_LAYOUT: LegendLayoutResult = { | ||
| effectiveType: LegendType.Scroll, | ||
| }; | ||
|
|
||
| function getLegendLabel(item: LegendDataItem): string { | ||
| if (typeof item === 'string' || typeof item === 'number') { | ||
| return String(item); | ||
|
|
@@ -269,37 +263,30 @@ function getHorizontalPlainLegendLayout({ | |
| showSelectors, | ||
| theme, | ||
| ); | ||
| const rowsForMargin = Number.isFinite(rowCount) | ||
| ? rowCount | ||
| : legendLabels.length; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a single label is wider than the available width,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, a finite margin does not mean the label fits. The fallback keeps the arithmetic sane but leaves the text unconstrained, so it can still paint outside the viewport. Truncating it properly means adding a width to the layout result and applying textStyle.width plus overflow in the legend builder for the 8 charts that use this helper (Radar, Gantt, Bubble, Pie, Timeseries, MixedTimeseries, Graph, Funnel). That about doubles this diff and pulls in six chart types unrelated to the original bug, so I would rather file it separately and pick it up next. Say the word if you want it here instead. |
||
| const requiredMargin = | ||
| defaultLegendPadding[orientation] + | ||
| Math.max(0, rowCount - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT; | ||
| const maxLegendHeight = | ||
| Math.max(0, rowsForMargin - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT; | ||
|
Comment on lines
+266
to
+271
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: When row estimation overflows ( Severity Level: Major
|
||
| const boundedMargin = | ||
| availableHeight > 0 | ||
| ? availableHeight * LEGEND_HORIZONTAL_MAX_HEIGHT_RATIO | ||
| : Infinity; | ||
|
|
||
| if ( | ||
| !Number.isFinite(rowCount) || | ||
| rowCount > LEGEND_HORIZONTAL_MAX_ROWS || | ||
| requiredMargin > maxLegendHeight | ||
| ) { | ||
| return SCROLL_LEGEND_LAYOUT; | ||
| } | ||
| ? Math.min(requiredMargin, availableHeight * MAX_LEGEND_MARGIN_RATIO) | ||
| : requiredMargin; | ||
|
|
||
| return { | ||
| effectiveMargin: Math.max(currentMargin, requiredMargin), | ||
| effectiveMargin: Math.max(currentMargin, boundedMargin), | ||
| effectiveType: LegendType.Plain, | ||
| }; | ||
| } | ||
|
|
||
| function getVerticalPlainLegendLayout({ | ||
| availableHeight, | ||
| availableWidth, | ||
| currentMargin, | ||
| legendLabels, | ||
| showSelectors, | ||
| theme, | ||
| }: { | ||
| availableHeight: number; | ||
| availableWidth: number; | ||
| currentMargin: number; | ||
| legendLabels: string[]; | ||
|
|
@@ -313,17 +300,6 @@ function getVerticalPlainLegendLayout({ | |
| }; | ||
| } | ||
|
|
||
| const selectorHeight = showSelectors | ||
| ? LEGEND_VERTICAL_ROW_HEIGHT + LEGEND_SELECTOR_GAP | ||
| : 0; | ||
| const effectiveAvailableHeight = Math.max( | ||
| availableHeight - LEGEND_VERTICAL_SIDE_GUTTER - selectorHeight, | ||
| 0, | ||
| ); | ||
| const rowsPerColumn = Math.floor( | ||
| (effectiveAvailableHeight + DEFAULT_LEGEND_ITEM_GAP) / | ||
| (LEGEND_VERTICAL_ROW_HEIGHT + DEFAULT_LEGEND_ITEM_GAP), | ||
| ); | ||
| const requiredSelectorMargin = showSelectors | ||
| ? ESTIMATED_LEGEND_SELECTOR_WIDTH + LEGEND_VERTICAL_SIDE_GUTTER | ||
| : 0; | ||
|
|
@@ -333,21 +309,13 @@ function getVerticalPlainLegendLayout({ | |
| requiredSelectorMargin, | ||
| ), | ||
| ); | ||
| const maxLegendWidth = | ||
| const boundedMargin = | ||
| availableWidth > 0 | ||
| ? availableWidth * LEGEND_VERTICAL_MAX_WIDTH_RATIO | ||
| : Infinity; | ||
|
|
||
| if ( | ||
| rowsPerColumn <= 0 || | ||
| legendLabels.length > rowsPerColumn || | ||
| requiredMargin > maxLegendWidth | ||
| ) { | ||
| return SCROLL_LEGEND_LAYOUT; | ||
| } | ||
| ? Math.min(requiredMargin, availableWidth * MAX_LEGEND_MARGIN_RATIO) | ||
| : requiredMargin; | ||
|
|
||
| return { | ||
| effectiveMargin: Math.max(currentMargin, requiredMargin), | ||
| effectiveMargin: Math.max(currentMargin, boundedMargin), | ||
| effectiveType: LegendType.Plain, | ||
| }; | ||
| } | ||
|
|
@@ -404,7 +372,6 @@ export function getLegendLayoutResult({ | |
| } | ||
|
|
||
| return getVerticalPlainLegendLayout({ | ||
| availableHeight: resolvedAvailableHeight, | ||
| availableWidth: resolvedAvailableWidth, | ||
| currentMargin: resolvedLegendMargin, | ||
| legendLabels, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -852,7 +852,7 @@ describe('Bar Chart X-axis Time Formatting', () => { | |
| type: LegendType.Plain, | ||
| }); | ||
|
|
||
| test('should fall back to scroll for horizontal bottom legends after margin expansion reduces available width', () => { | ||
| test('honors an explicit List selection for horizontal bottom legends and reserves margin', () => { | ||
| const legendLabels = [ | ||
| 'This is a long sales legend', | ||
| 'This is a long marketing legend', | ||
|
|
@@ -899,64 +899,30 @@ describe('Bar Chart X-axis Time Formatting', () => { | |
| legendType: LegendType.Plain, | ||
| showLegend: true, | ||
| }; | ||
| const baselineChartProps = createEchartsTimeseriesTestChartProps< | ||
| EchartsTimeseriesFormData, | ||
| EchartsTimeseriesChartProps | ||
| >({ | ||
| defaultFormData: regressionFormData, | ||
| defaultVizType: 'echarts_timeseries_bar', | ||
| defaultQueriesData: longLegendData, | ||
| width: baseChartPropsConfig.width, | ||
| height: baseChartPropsConfig.height, | ||
| }); | ||
| const baselineTransformed = transformProps(baselineChartProps); | ||
| const legendItems = ( | ||
| (baselineTransformed.echartOptions.legend as LegendComponentOption) | ||
| .data as Array<string | { name: string }> | ||
| ).map(item => (typeof item === 'string' ? item : item.name)); | ||
| let chartWidth: number | undefined; | ||
| let expandedLegendMargin: number | null = null; | ||
|
|
||
| for (let width = 300; width <= 700; width += 1) { | ||
| const initialLayout = getBottomLegendLayout(width, legendItems, null); | ||
|
|
||
| if (initialLayout.effectiveType !== LegendType.Plain) { | ||
| continue; | ||
| } | ||
|
|
||
| const refinedLayout = getBottomLegendLayout( | ||
| width, | ||
| legendItems, | ||
| initialLayout.effectiveMargin ?? null, | ||
| ); | ||
|
|
||
| if (refinedLayout.effectiveType === LegendType.Scroll) { | ||
| chartWidth = width; | ||
| expandedLegendMargin = initialLayout.effectiveMargin ?? null; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| expect(chartWidth).toBeDefined(); | ||
| expect(expandedLegendMargin).not.toBeNull(); | ||
| const resolvedChartWidth = chartWidth ?? baseChartPropsConfig.width; | ||
|
|
||
| // A narrow chart forces the long-label bottom legend to wrap onto | ||
| // multiple rows — the case that previously flipped List to scroll. | ||
| const chartWidth = 320; | ||
| const chartProps = createEchartsTimeseriesTestChartProps< | ||
| EchartsTimeseriesFormData, | ||
| EchartsTimeseriesChartProps | ||
| >({ | ||
| defaultFormData: regressionFormData, | ||
| defaultVizType: 'echarts_timeseries_bar', | ||
| defaultQueriesData: longLegendData, | ||
| width: resolvedChartWidth, | ||
| width: chartWidth, | ||
| height: baseChartPropsConfig.height, | ||
| }); | ||
|
|
||
| const transformedProps = transformProps(chartProps); | ||
| const legend = transformedProps.echartOptions | ||
| .legend as LegendComponentOption; | ||
| const grid = transformedProps.echartOptions.grid as GridComponentOption; | ||
| const expectedPadding = getPadding( | ||
| const legendItems = (legend.data as Array<string | { name: string }>).map( | ||
| item => (typeof item === 'string' ? item : item.name), | ||
| ); | ||
|
|
||
| const layout = getBottomLegendLayout(chartWidth, legendItems, null); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The layout helper is invoked with a null legend type, so this assertion path is not actually exercising the explicit List/Plain selection scenario described by the test. This can let a regression slip through because the helper call is testing default behavior instead of the user-selected type; pass the explicit plain type to the helper in this check. [logic error] Severity Level: Major
|
||
| const basePadding = getPadding( | ||
| true, | ||
| LegendOrientation.Bottom, | ||
| false, | ||
|
|
@@ -968,30 +934,41 @@ describe('Bar Chart X-axis Time Formatting', () => { | |
| undefined, | ||
| true, | ||
| ); | ||
| [expectedPadding.bottom, expectedPadding.left] = [ | ||
| expectedPadding.left, | ||
| expectedPadding.bottom, | ||
| [basePadding.bottom, basePadding.left] = [ | ||
| basePadding.left, | ||
| basePadding.bottom, | ||
| ]; | ||
|
Comment on lines
+937
to
940
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swapped padding corrupts baseline
The destructuring-swap Code Review Run #12858b Should Bito avoid suggestions like this for future reviews? (Manage Rules)
|
||
| const expandedPadding = getPadding( | ||
|
|
||
| // The explicit List selection is honored end-to-end (never flips). | ||
| expect(legend.type).toBe(LegendType.Plain); | ||
| expect(layout.effectiveType).toBe(LegendType.Plain); | ||
|
|
||
| // #38675's margin reservation is retained: the wrapped rows reserve a | ||
| // finite margin beyond the single-row baseline, so the grid shrinks to | ||
| // reduce clipping instead of the legend flipping to scroll. | ||
| expect(Number.isFinite(layout.effectiveMargin)).toBe(true); | ||
|
|
||
| const reservedPadding = getPadding( | ||
| true, | ||
| LegendOrientation.Bottom, | ||
| false, | ||
| false, | ||
| expandedLegendMargin, | ||
| layout.effectiveMargin, | ||
| false, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| true, | ||
| ); | ||
| [expandedPadding.bottom, expandedPadding.left] = [ | ||
| expandedPadding.left, | ||
| expandedPadding.bottom, | ||
| [reservedPadding.bottom, reservedPadding.left] = [ | ||
| reservedPadding.left, | ||
| reservedPadding.bottom, | ||
| ]; | ||
|
Comment on lines
+963
to
966
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Second swap on reservedPadding
The second destructuring-swap on lines 963-966 mutates Code Review Run #12858b Should Bito avoid suggestions like this for future reviews? (Manage Rules)
|
||
|
|
||
| expect(legend.type).toBe(LegendType.Scroll); | ||
| expect(grid.bottom).toBe(expectedPadding.bottom); | ||
| expect(grid.bottom).not.toBe(expandedPadding.bottom); | ||
| expect(grid.bottom).toBe(reservedPadding.bottom); | ||
| expect(grid.bottom as number).toBeGreaterThan( | ||
| basePadding.bottom as number, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
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.
Suggestion: The comment says the cap is only for horizontal legends, but the same constant is now used for both horizontal and vertical layout code paths; this mismatch is misleading and can cause incorrect future maintenance changes. Update the comment to reflect both orientations or split constants/comments per orientation. [comment mismatch]
Severity Level: Minor 🧹
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖