jesperct commented on code in PR #41749:
URL: https://github.com/apache/superset/pull/41749#discussion_r3632751815
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -344,7 +344,7 @@ describe('legend sorting', () => {
const result = transformProps(props as EchartsGanttChartProps);
- expect((result.echartOptions.legend as any).type).toBe(LegendType.Scroll);
+ expect((result.echartOptions.legend as any).type).toBe(LegendType.Plain);
Review Comment:
Fixed on 14 Jul in f7e1080bd4. That assertion uses LegendComponentOption now
instead of the any cast.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -269,21 +259,12 @@ function getHorizontalPlainLegendLayout({
showSelectors,
theme,
);
+ const rowsForMargin = Number.isFinite(rowCount)
+ ? rowCount
+ : legendLabels.length;
const requiredMargin =
defaultLegendPadding[orientation] +
- Math.max(0, rowCount - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT;
- const maxLegendHeight =
- 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.max(0, rowsForMargin - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT;
Review Comment:
Valid, and it took two passes to fully close. The horizontal path got a cap
in 685fd8cc41 (40% of the chart height). The vertical path had the same flaw
and went unnoticed until a reviewer raised it, fixed today in 56f1ba42bb. Both
now clamp against the available dimension.
##########
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 in 56f1ba42bb. The assertion derives the expected padding from
layout.effectiveMargin and asserts grid.bottom matches it exactly, plus a
strict greater-than over the single row baseline.
##########
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);
Review Comment:
I do not think this one holds. The third parameter of getBottomLegendLayout
is legendMargin (line 826), not the legend type, and the helper hardcodes type:
LegendType.Plain in the options it builds (line 852). Passing null means no
user-set margin, so the default padding applies, which is the case this test is
meant to cover. The explicit Plain selection is still exercised end to end.
--
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]
