This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch viz-pipeline-followups in repository https://gitbox.apache.org/repos/asf/superset.git
commit 82f05043f9db18ee0a77f28c4ed3e28aeafb7e80 Author: Claude Code <[email protected]> AuthorDate: Tue Jul 28 00:55:31 2026 -0700 fix(time-pivot): extract Bullet's legend-row estimator, fix a shared overlap bug Bullet's dynamic legend-row estimation (added to fix a legend-overlapping- plot bug) was local to its own transformProps.ts. TimePivot has the exact same class of bug: with period_limit unset a chart can have 50+ series (current + many priors), and its grid.top was a fixed constant regardless of legend size, so a wide legend can wrap onto extra rows with nothing reserving space for them. Extracted the estimator into utils/legendLayout.ts as estimateWrappedLegendRowCount() (also naming its previously-inline magic numbers), reused it from Bullet, and wired it into TimePivot's grid.top so extra wrapped rows get extra height instead of overlapping the plot. TimePivot's single-row case is unchanged (same fixed padding as before). Added tests for the shared helper itself, and for TimePivot: a large legend reserves more grid.top than a small one, and a hidden legend reserves none of the extra space. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/Bullet/transformProps.ts | 31 ++++----- .../src/TimePivot/transformProps.ts | 22 ++++++- .../src/utils/legendLayout.test.ts | 77 ++++++++++++++++++++++ .../plugin-chart-echarts/src/utils/legendLayout.ts | 40 +++++++++++ .../test/TimePivot/transformProps.test.ts | 73 ++++++++++++++++++++ 5 files changed, 222 insertions(+), 21 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts index 93b660089a2..cfa71e3b897 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts @@ -29,8 +29,12 @@ import type { EChartsCoreOption } from 'echarts/core'; import { Refs } from '../types'; import { EchartsBulletChartProps, BulletChartTransformedProps } from './types'; import { tokenizeToNumericArray, tokenizeToStringArray } from './utils'; +import { estimateWrappedLegendRowCount } from '../utils/legendLayout'; const MEASURE_BAR_FRACTION = 0.28; +// Small pixel gap below the measure bar so the target/marker symbol doesn't +// touch its edge. +const MARKER_GAP_BELOW_BAR_PX = 12; export default function transformProps( chartProps: EchartsBulletChartProps, @@ -157,26 +161,15 @@ export default function transformProps( ...markers.map((value, i) => markerName(value, i, markerLabels)), ...markerLines.map((value, i) => markerName(value, i, markerLineLabels)), ]; - const estimateLegendRows = (names: string[]): number => { - // legend icon width + icon-text gap + inter-item gap (ECharts defaults) - const itemOverhead = 25 + 5 + 10; - const avgCharWidth = theme.fontSize * 0.6; - const available = Math.max(width - theme.sizeUnit * 4, 1); - let rows = 1; - let cursor = 0; - names.forEach(name => { - const itemWidth = itemOverhead + name.length * avgCharWidth; - if (cursor > 0 && cursor + itemWidth > available) { - rows += 1; - cursor = 0; - } - cursor += itemWidth; - }); - return rows; - }; const legendRowHeight = theme.fontSize + theme.sizeUnit * 3; const gridTop = showLegend - ? estimateLegendRows(legendNames) * legendRowHeight + theme.sizeUnit * 2 + ? estimateWrappedLegendRowCount({ + names: legendNames, + availableWidth: width - theme.sizeUnit * 4, + theme, + }) * + legendRowHeight + + theme.sizeUnit * 2 : theme.sizeUnit * 2; // The measure bar spans MEASURE_BAR_FRACTION of the category band @@ -185,7 +178,7 @@ export default function transformProps( const gridHeight = Math.max(height - gridTop - theme.sizeUnit * 6, 40); const rowHeight = gridHeight / categories.length; const markerOffsetPx = Math.round( - (MEASURE_BAR_FRACTION / 2) * rowHeight + 12, + (MEASURE_BAR_FRACTION / 2) * rowHeight + MARKER_GAP_BELOW_BAR_PX, ); const echartOptions: EChartsCoreOption = { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/TimePivot/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/TimePivot/transformProps.ts index dc33d054204..408866315ee 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/TimePivot/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/TimePivot/transformProps.ts @@ -26,6 +26,7 @@ import { import type { EChartsCoreOption } from 'echarts/core'; import { Refs } from '../types'; import { calculateLowerLogTick } from '../utils/series'; +import { estimateWrappedLegendRowCount } from '../utils/legendLayout'; import transformData, { TimePivotSeries } from './transformData'; import { EchartsTimePivotChartProps, @@ -104,9 +105,26 @@ export default function transformProps( } } + const legendNames = series.map(s => s.key); + // A single-row legend fits the chart's established top padding as before; + // with `period_limit` unset a chart can have 50+ series (current + many + // priors), so a wider legend can wrap onto extra rows that would + // otherwise overlap the plot -- reserve additional height per extra row. + const legendRowHeight = theme.fontSize + theme.sizeUnit * 3; + const legendRows = + showLegend !== false + ? estimateWrappedLegendRowCount({ + names: legendNames, + availableWidth: width - theme.sizeUnit * 4, + theme, + }) + : 0; + const gridTop = + theme.sizeUnit * 8 + Math.max(legendRows - 1, 0) * legendRowHeight; + const echartOptions: EChartsCoreOption = { grid: { - top: theme.sizeUnit * 8, + top: gridTop, bottom: theme.sizeUnit * 8, left: theme.sizeUnit * 4, right: theme.sizeUnit * 6, @@ -115,7 +133,7 @@ export default function transformProps( legend: { show: showLegend !== false, top: 0, - data: series.map(s => s.key), + data: legendNames, }, xAxis: { type: 'time', diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.test.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.test.ts new file mode 100644 index 00000000000..d0155d18c8a --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.test.ts @@ -0,0 +1,77 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { supersetTheme } from '@apache-superset/core/theme'; +import { estimateWrappedLegendRowCount } from './legendLayout'; + +test('a single short name fits on one row', () => { + expect( + estimateWrappedLegendRowCount({ + names: ['A'], + availableWidth: 800, + theme: supersetTheme, + }), + ).toBe(1); +}); + +test('an empty legend still reports one row', () => { + expect( + estimateWrappedLegendRowCount({ + names: [], + availableWidth: 800, + theme: supersetTheme, + }), + ).toBe(1); +}); + +test('wraps onto more rows as names are added past the available width', () => { + const names = Array.from({ length: 30 }, (_, i) => `series-${i}`); + const rows = estimateWrappedLegendRowCount({ + names, + availableWidth: 400, + theme: supersetTheme, + }); + expect(rows).toBeGreaterThan(1); + + // The same names fit onto fewer (or equal) rows given more width. + const widerRows = estimateWrappedLegendRowCount({ + names, + availableWidth: 4000, + theme: supersetTheme, + }); + expect(widerRows).toBeLessThanOrEqual(rows); +}); + +test('longer names wrap sooner than shorter ones at the same width', () => { + const shortNames = Array.from({ length: 10 }, (_, i) => `${i}`); + const longNames = Array.from( + { length: 10 }, + (_, i) => `a much longer series label ${i}`, + ); + const shortRows = estimateWrappedLegendRowCount({ + names: shortNames, + availableWidth: 500, + theme: supersetTheme, + }); + const longRows = estimateWrappedLegendRowCount({ + names: longNames, + availableWidth: 500, + theme: supersetTheme, + }); + expect(longRows).toBeGreaterThan(shortRows); +}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.ts index 200efc71b98..77a1c790214 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.ts @@ -29,6 +29,46 @@ export type ResolvedLegendLayout = { legendLayout: LegendLayoutResult; }; +// ECharts default spacing for a horizontal, top-aligned legend: the icon +// itself, the gap between icon and label, and the gap before the next item. +const LEGEND_ITEM_ICON_WIDTH = 25; +const LEGEND_ITEM_ICON_GAP = 5; +const LEGEND_ITEM_INTER_GAP = 10; +const LEGEND_ITEM_OVERHEAD = + LEGEND_ITEM_ICON_WIDTH + LEGEND_ITEM_ICON_GAP + LEGEND_ITEM_INTER_GAP; + +/** + * Estimates how many rows a horizontal, top-aligned legend will wrap onto + * for a given chart width, so callers can reserve enough `grid.top` space to + * avoid the legend overlapping the plot. ECharts only reports this after a + * render, so this is a best-effort estimate from item label lengths using + * the library's default per-item spacing; it degrades gracefully (a slight + * under/over-reservation) rather than needing an extra render pass. + */ +export function estimateWrappedLegendRowCount({ + names, + availableWidth, + theme, +}: { + names: string[]; + availableWidth: number; + theme: SupersetTheme; +}): number { + const avgCharWidth = theme.fontSize * 0.6; + const available = Math.max(availableWidth, 1); + let rows = 1; + let cursor = 0; + names.forEach(name => { + const itemWidth = LEGEND_ITEM_OVERHEAD + name.length * avgCharWidth; + if (cursor > 0 && cursor + itemWidth > available) { + rows += 1; + cursor = 0; + } + cursor += itemWidth; + }); + return rows; +} + export function resolveLegendLayout(args: { availableHeight?: number; availableWidth?: number; diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/TimePivot/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/TimePivot/transformProps.test.ts index a83d261d1e9..440fe3cbac7 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/TimePivot/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/TimePivot/transformProps.test.ts @@ -78,6 +78,79 @@ test('honors log scale and y-axis bounds', () => { expect(yAxis.max).toBe(100); }); +test('reserves extra grid.top space when a large legend wraps onto more rows', () => { + // Unbounded period_limit: 25 weekly records -> 25 periods (current + 24 + // priors), whose short "-N" labels still wrap a legend onto 2+ rows at + // this chart width -- the exact scenario left unaddressed by Bullet's + // dynamic legend-row estimation fix (this chart used a fixed grid.top + // regardless of legend size). + const manyWeeks = Array.from({ length: 25 }, (_, i) => ({ + __timestamp: MONDAY_1 + i * WEEK, + sum__num: i, + })); + const singleWeek = [ + { __timestamp: MONDAY_1, sum__num: 10 }, + { __timestamp: MONDAY_2, sum__num: 20 }, + ]; + + const baseline = transformProps( + new ChartProps({ + width: 800, + height: 400, + formData, + theme: supersetTheme, + queriesData: [{ data: singleWeek }], + hooks: {}, + }) as unknown as EchartsTimePivotChartProps, + ); + const manySeries = transformProps( + new ChartProps({ + width: 800, + height: 400, + formData, + theme: supersetTheme, + queriesData: [{ data: manyWeeks }], + hooks: {}, + }) as unknown as EchartsTimePivotChartProps, + ); + + const baselineTop = (baseline.echartOptions as any).grid.top; + const manySeriesTop = (manySeries.echartOptions as any).grid.top; + expect(manySeriesTop).toBeGreaterThan(baselineTop); +}); + +test('does not reserve extra grid.top space for a large legend when it is hidden', () => { + const manyWeeks = Array.from({ length: 25 }, (_, i) => ({ + __timestamp: MONDAY_1 + i * WEEK, + sum__num: i, + })); + + const shown = transformProps( + new ChartProps({ + width: 800, + height: 400, + formData: { ...formData, showLegend: true }, + theme: supersetTheme, + queriesData: [{ data: manyWeeks }], + hooks: {}, + }) as unknown as EchartsTimePivotChartProps, + ); + const hidden = transformProps( + new ChartProps({ + width: 800, + height: 400, + formData: { ...formData, showLegend: false }, + theme: supersetTheme, + queriesData: [{ data: manyWeeks }], + hooks: {}, + }) as unknown as EchartsTimePivotChartProps, + ); + + expect((hidden.echartOptions as any).grid.top).toBeLessThan( + (shown.echartOptions as any).grid.top, + ); +}); + test('handles an empty result without crashing', () => { const props = new ChartProps({ width: 800,
