sadpandajoe commented on code in PR #41749:
URL: https://github.com/apache/superset/pull/41749#discussion_r3625905601
##########
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:
This computed margin can exceed the chart width for long left/right labels,
leaving no usable plot area. Could we pass the available width into this path,
cap the automatic vertical margin as the horizontal path does, and cover it
with a long-label regression?
##########
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:
`>=` allows the single-row baseline, so this passes even if the wrapped
legend margin stops reaching the final grid. Could this assert a strict
increase, or the exact padding derived from `layout.effectiveMargin`?
##########
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:
This test name includes an internal ticket identifier in public source.
Could we remove it or replace it with a public GitHub issue reference?
##########
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:
This only proves the margin is below 100% of the chart height, so changing
the intended 40% cap to 99% would still pass. Could we assert `chartHeight *
0.4` (80px here) directly?
##########
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:
When a single label is wider than the available width, `rowCount` is
infinite but this still preserves Plain without constraining the text, so
ECharts renders outside the viewport. Could we bound or truncate oversized
horizontal labels while keeping the explicit List selection?
--
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]
