jesperct commented on code in PR #41749:
URL: https://github.com/apache/superset/pull/41749#discussion_r3632386585
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -329,18 +303,6 @@ function getVerticalPlainLegendLayout({
requiredSelectorMargin,
),
);
- const maxLegendWidth =
- availableWidth > 0
- ? availableWidth * LEGEND_VERTICAL_MAX_WIDTH_RATIO
- : Infinity;
-
- if (
- rowsPerColumn <= 0 ||
- legendLabels.length > rowsPerColumn ||
- requiredMargin > maxLegendWidth
- ) {
- return SCROLL_LEGEND_LAYOUT;
- }
return {
effectiveMargin: Math.max(currentMargin, requiredMargin),
Review Comment:
The vertical path had no cap at all. It reserved whatever the longest label
measured, so one long series name could ask for more width than the chart has.
It now takes availableWidth and applies the same 40% cap as the horizontal
path. Test written first: 1533px reserved on a 1000px chart before, 400px after.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -265,38 +259,29 @@ function getHorizontalPlainLegendLayout({
showSelectors,
theme,
);
+ const rowsForMargin = Number.isFinite(rowCount)
+ ? rowCount
+ : legendLabels.length;
Review Comment:
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.
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -1073,86 +1073,116 @@ test('getLegendLayoutResult adds extra margin for
wrapped plain horizontal legen
);
});
-test('getLegendLayoutResult falls back to scroll when horizontal plain legends
exceed two rows', () => {
- expect(
- getLegendLayoutResult({
- chartHeight: 400,
- chartWidth: 240,
- legendItems: [
- 'This is a long legend label',
- 'Another long legend label',
- 'Third long legend label',
- ],
- legendMargin: null,
- orientation: LegendOrientation.Top,
- show: true,
- theme,
- type: LegendType.Plain,
- }),
- ).toEqual({
- effectiveType: LegendType.Scroll,
+test('getLegendLayoutResult keeps plain when horizontal plain legends exceed
two rows', () => {
+ const layout = getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 240,
+ legendItems: [
+ 'This is a long legend label',
+ 'Another long legend label',
+ 'Third long legend label',
+ ],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
});
+
+ expect(layout.effectiveType).toBe(LegendType.Plain);
+ expect(layout.effectiveMargin).toEqual(expect.any(Number));
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+ expect(layout.effectiveMargin).toBeGreaterThanOrEqual(
+ defaultLegendPadding[LegendOrientation.Top],
+ );
});
-test('getLegendLayoutResult falls back to scroll when a single horizontal
plain legend item exceeds available width', () => {
- expect(
- getLegendLayoutResult({
- chartHeight: 400,
- chartWidth: 260,
- legendItems: [
- 'This is a ridiculously long legend label that should not fit on one
line',
- ],
- legendMargin: null,
- orientation: LegendOrientation.Top,
- show: true,
- theme,
- type: LegendType.Plain,
- }),
- ).toEqual({
- effectiveType: LegendType.Scroll,
+test('getLegendLayoutResult bounds reserved margin for overflowing horizontal
legends so the plot is not collapsed', () => {
+ const chartHeight = 200;
+ const layout = getLegendLayoutResult({
+ chartHeight,
+ chartWidth: 100,
+ legendItems: Array.from({ length: 100 }, (_, index) => `Series ${index}`),
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
});
+
+ expect(layout.effectiveType).toBe(LegendType.Plain);
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+ expect(layout.effectiveMargin as number).toBeLessThan(chartHeight);
Review Comment:
Fixed, it asserts 80 directly now, which is the 40% cap on the 200px chart
height. Changing the ratio fails the test.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -899,99 +899,57 @@ 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(
- true,
- LegendOrientation.Bottom,
- false,
- false,
- null,
- false,
- undefined,
- undefined,
- undefined,
- true,
+ const legendItems = (legend.data as Array<string | { name: string
}>).map(
+ item => (typeof item === 'string' ? item : item.name),
);
- [expectedPadding.bottom, expectedPadding.left] = [
- expectedPadding.left,
- expectedPadding.bottom,
- ];
- const expandedPadding = getPadding(
+
+ const layout = getBottomLegendLayout(chartWidth, legendItems, null);
+ const basePadding = getPadding(
true,
LegendOrientation.Bottom,
false,
false,
- expandedLegendMargin,
+ null,
false,
undefined,
undefined,
undefined,
true,
);
- [expandedPadding.bottom, expandedPadding.left] = [
- expandedPadding.left,
- expandedPadding.bottom,
+ [basePadding.bottom, basePadding.left] = [
+ basePadding.left,
+ basePadding.bottom,
];
- expect(legend.type).toBe(LegendType.Scroll);
- expect(grid.bottom).toBe(expectedPadding.bottom);
- expect(grid.bottom).not.toBe(expandedPadding.bottom);
+ // 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(layout.effectiveMargin).toEqual(expect.any(Number));
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+ expect(grid.bottom as number).toBeGreaterThanOrEqual(
Review Comment:
Fixed. It derives the expected padding from layout.effectiveMargin and
asserts grid.bottom matches it exactly, plus a strict greater-than against the
single row baseline.
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -1193,56 +1223,93 @@ test('getLegendLayoutResult adds extra margin for wide
vertical plain legends',
);
});
-test('getLegendLayoutResult falls back to scroll when vertical plain legends
exceed one column', () => {
- expect(
- getLegendLayoutResult({
- chartHeight: 160,
- chartWidth: 800,
- legendItems: ['Alpha', 'Beta', 'Gamma', 'Delta', 'Epsilon'],
- legendMargin: null,
- orientation: LegendOrientation.Left,
- show: true,
- theme,
- type: LegendType.Plain,
- }),
- ).toEqual({
- effectiveType: LegendType.Scroll,
+test('getLegendLayoutResult keeps plain when vertical plain legends exceed one
column', () => {
+ const layout = getLegendLayoutResult({
+ chartHeight: 160,
+ chartWidth: 800,
+ legendItems: ['Alpha', 'Beta', 'Gamma', 'Delta', 'Epsilon'],
+ legendMargin: null,
+ orientation: LegendOrientation.Left,
+ show: true,
+ theme,
+ type: LegendType.Plain,
});
+
+ expect(layout.effectiveType).toBe(LegendType.Plain);
+ expect(layout.effectiveMargin).toEqual(expect.any(Number));
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+ expect(layout.effectiveMargin).toBeGreaterThanOrEqual(
+ defaultLegendPadding[LegendOrientation.Left],
+ );
});
-test('getLegendLayoutResult falls back to scroll when vertical plain legend
selectors exceed available width', () => {
- expect(
- getLegendLayoutResult({
- chartHeight: 400,
- chartWidth: 300,
- legendItems: ['A', 'B', 'C'],
- legendMargin: null,
- orientation: LegendOrientation.Left,
- show: true,
- theme,
- type: LegendType.Plain,
- }),
- ).toEqual({
- effectiveType: LegendType.Scroll,
+test('getLegendLayoutResult keeps plain when vertical plain legend selectors
exceed available width', () => {
+ const layout = getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 300,
+ legendItems: ['A', 'B', 'C'],
+ legendMargin: null,
+ orientation: LegendOrientation.Left,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ });
+
+ expect(layout.effectiveType).toBe(LegendType.Plain);
+ expect(layout.effectiveMargin).toEqual(expect.any(Number));
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+ expect(layout.effectiveMargin).toBeGreaterThanOrEqual(
+ defaultLegendPadding[LegendOrientation.Left],
+ );
+});
+
+test('getLegendLayoutResult honors an explicit List selection with many series
(SC-101478)', () => {
Review Comment:
Removed. I rewrote the commit that introduced it and force pushed, so it is
out of the branch history rather than just fixed on top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
