diff --git a/.changeset/7248-scatter-legend-names-its-series.md b/.changeset/7248-scatter-legend-names-its-series.md new file mode 100644 index 0000000000..fa76eda861 --- /dev/null +++ b/.changeset/7248-scatter-legend-names-its-series.md @@ -0,0 +1,31 @@ +--- +'@object-ui/plugin-charts': patch +--- + +Name the scatter legend's series, so its swatch stops reading as a stray data point +(objectui#7248). + +The Chart Gallery scatter ("Estimate vs Progress") appeared to draw a seventh point +below the x-axis, outside the plot area. It was not a point. `ChartLegendContent` +resolves a label as `config[nameKey || item.dataKey || 'value']`, and a `` +carries **no `dataKey`** — scatter's keys live on the XAxis/YAxis, not on the mark — so +the key collapsed to the literal string `'value'`, missed a config keyed by measure +name, and the legend entry rendered its colour swatch with no text beside it. An 8x8 +square in `--chart-1`, the same colour as the marks, sitting under the x-axis. + +Measured on the running showcase in real Chromium: the swatch sat at cy 341 against a +plot area ending at cy 295, on a y scale of 4.835 px per unit — y = -9.5, at x ≈ 45. +That is the "x≈40, y≈-10" the report described, to the pixel, and all six real marks +were inside the plot area at every viewport width swept from 1440 down to 480. + +**The y domain was not the defect and is unchanged.** Clamping it — the fix the report +asked for — would have created the bug it described: mixed-sign and all-negative +fixtures are pinned here drawing every mark, because recharts already extends the +domain to cover negative values. + +Two changes. The scatter now passes `nameKey` so its legend resolves the measure's +label, and `ChartLegendContent` falls back to the series `name` recharts itself put on +the legend item when the config lookup misses. The second closes the class rather than +this one instance: the swatch renders unconditionally, so a config miss must never +leave it anonymous. Charts whose config already resolves are unaffected — only a +currently-empty label changes. diff --git a/packages/plugin-charts/src/AdvancedChartImpl.scatterLegendLabel.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.scatterLegendLabel.test.tsx new file mode 100644 index 0000000000..8002c6788d --- /dev/null +++ b/packages/plugin-charts/src/AdvancedChartImpl.scatterLegendLabel.test.tsx @@ -0,0 +1,197 @@ +/** + * objectui#7248 — the scatter legend rendered a colour swatch with NO label, + * and that anonymous dot read as a DATA POINT drawn outside the plot area. + * + * ## The card said "y domain", and the y domain was innocent + * + * The card reported the Chart Gallery scatter ("Estimate vs Progress") drawing + * "a seventh point below the x-axis at x≈40, y≈-10" and asked for the y domain + * to include every plotted value. Measured on the running showcase in real + * Chromium (viewport 1440, widget svg 510x350) that diagnosis is FALSE and the + * fix would have been in the wrong place: + * + * - all SIX marks sat inside the plot area (y 5..295): cy 215.3, 101.7, 5, + * 179, 150, 237. Nothing was outside it, at any viewport width swept from + * 1440 down to 480. + * - the seventh "point" was the LEGEND swatch — an 8x8 `--chart-1` square, + * the same colour as the marks, at cx 255 / cy 341, with `innerText === ""`. + * - the arithmetic closes it: the plot bottom (y=0) is cy 295 and the scale + * is 4.835 px per unit, so cy 341 is y = -9.5 and cx 255 is x ≈ 45 — + * the card's "x≈40, y≈-10", to the pixel. + * + * The domain is therefore NOT touched, and the two fixtures that would expose a + * real domain bug are pinned below instead: mixed-sign and all-negative data + * both draw every mark, because recharts extends the domain to cover negatives. + * + * ## Why the label was empty + * + * `ChartLegendContent` resolves a label as `config[nameKey || item.dataKey || + * 'value']`. A `` carries NO `dataKey` — scatter's keys live on the + * XAxis/YAxis, not on the mark — so the key collapsed to the literal string + * `'value'` and missed a config keyed by measure name. Captured from recharts + * 3.10.1 rather than assumed, the scatter's legend payload item is: + * + * { color: '#3b82f6', type: 'circle', value: 'Avg Estimate', + * payload: { name: 'Avg Estimate', ... } } // NO `dataKey` + * + * The name was right there in `value` the whole time; the legend just never + * read it. Both halves of the fix are pinned here: the scatter now passes + * `nameKey`, and `ChartLegendContent` falls back to `item.value` so that NO + * family can render a nameless swatch again. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import React from 'react'; +import { render, cleanup } from '@testing-library/react'; + +vi.mock('recharts', async () => { + const actual = await vi.importActual('recharts'); + return { + ...actual, + ResponsiveContainer: ({ children }: any) => + React.cloneElement(children, { width: 480, height: 320 }), + }; +}); + +import AdvancedChartImpl from './AdvancedChartImpl'; + +afterEach(cleanup); + +type Row = Record; + +/** The six aggregate rows the Chart Gallery scatter actually plots, read from + * the running showcase API: progress buckets x avg estimate_hours. */ +const GALLERY: Row[] = [ + { progress: 0, avg_estimate: 16.5 }, + { progress: 45, avg_estimate: 40 }, + { progress: 55, avg_estimate: 60 }, + { progress: 80, avg_estimate: 24 }, + { progress: 90, avg_estimate: 30 }, + { progress: 100, avg_estimate: 12 }, +]; + +const CONFIG = { progress: { label: 'Progress' }, avg_estimate: { label: 'Avg Estimate' } }; + +const renderScatter = (props: Record = {}) => + render( + , + ); + +/** + * One entry per legend item: does it paint a swatch, and what text sits beside + * it. The swatch is the `h-2 w-2` div `ChartLegendContent` renders when the + * config carries no icon — the exact element measured at cy 341 on screen. + */ +const legendEntries = (c: HTMLElement) => { + const wrapper = c.querySelector('.recharts-legend-wrapper'); + const row = wrapper?.firstElementChild; + return [...(row?.children ?? [])].map((el) => ({ + hasSwatch: !!el.querySelector('div.h-2.w-2'), + text: (el.textContent ?? '').trim(), + })); +}; + +const marksOf = (c: HTMLElement) => c.querySelectorAll('path.recharts-symbols').length; + +describe('objectui#7248 — the scatter legend names its series', () => { + it('renders the measure label beside the swatch, not a bare dot', () => { + // The defect, stated as the reader sees it: a swatch with nothing next to + // it is indistinguishable from a data point, and this one was painted + // below the x-axis in the marks' own colour. + const { container } = renderScatter(); + expect(legendEntries(container)).toEqual([{ hasSwatch: true, text: 'Avg Estimate' }]); + }); + + it('falls back to the series name when the config has no entry for the measure', () => { + // `nameKey` alone would leave the hole open one config-miss away, which is + // how this bug survived the pie fix that closed the same shape in #3135. + // + // The fallback lands on the RAW measure key, not a prose label, because + // with no config entry the scatter's own `name` is `s.dataKey` — that is + // the honest reading and it is still the point: `avg_estimate` identifies + // the series, an unlabelled dot does not. + const { container } = renderScatter({ config: { progress: { label: 'Progress' } } }); + expect(legendEntries(container)).toEqual([{ hasSwatch: true, text: 'avg_estimate' }]); + }); + + it('resolves the legend entry through the CONFIG, not just the series name', () => { + // What `nameKey` buys, pinned separately because the `item.value` fallback + // masks it on the label: with the key collapsed to `'value'` the config + // lookup can never hit, so `itemConfig.icon` is dead for scatter and a + // config-declared legend icon is silently ignored — while every other + // family honours it. Measured via the icon because that is the one branch + // the fallback cannot stand in for. + const Icon = () => ; + const { container } = renderScatter({ + config: { progress: { label: 'Progress' }, avg_estimate: { label: 'Avg Estimate', icon: Icon } }, + }); + expect(container.querySelector('[data-testid="legend-icon"]')).not.toBeNull(); + expect(legendEntries(container)).toEqual([{ hasSwatch: false, text: 'Avg Estimate' }]); + }); + + it('never paints a swatch with no text — the invariant, across families', () => { + // The class this card belongs to, not just its one instance. A legend entry + // that cannot be named is worse than no legend: it adds an anonymous mark + // to a picture whose whole job is to be read. + for (const chartType of ['scatter', 'bar', 'line', 'area'] as const) { + const { container } = render( + , + ); + const nameless = legendEntries(container).filter((e) => e.hasSwatch && e.text === ''); + expect(nameless, `${chartType} painted an unlabelled legend swatch`).toEqual([]); + cleanup(); + } + }); +}); + +describe('objectui#7248 — the y domain was NOT the defect, and stays untouched', () => { + it('draws every gallery row', () => { + // The control. Six aggregate rows, six marks — measured in Chromium as six + // marks all inside the plot area, so there is no missing/expelled point. + expect(marksOf(renderScatter().container)).toBe(6); + }); + + it('draws every mark for mixed-sign and all-negative data', () => { + // The two fixtures a real y-domain bug would fail. Recharts extends the + // domain to cover negatives, so clamping it to `[0, 'auto']` here — the fix + // the card asked for — would have CREATED the defect it described. + const mixed = render( + , + ); + expect(marksOf(mixed.container)).toBe(3); + cleanup(); + + const allNeg = render( + , + ); + expect(marksOf(allNeg.container)).toBe(2); + }); + + it('leaves the #7197 unplaceable-row notice alone', () => { + // The card asked whether this was #7197's job. It is not: that notice + // answers rows with NO number, and every gallery row has two. + const { container } = renderScatter(); + expect(container.querySelector('[data-chart-note="unplotted-points"]')).toBeNull(); + expect(container.querySelector('[data-chart-error="no-plottable-points"]')).toBeNull(); + }); +}); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index 2acc581d3f..640e229054 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -1728,8 +1728,28 @@ function AdvancedChartImplInner({ /> } /> + {/* `nameKey` is REQUIRED here, for a reason unique to scatter + (objectui#7248). `ChartLegendContent` resolves a label as + `config[nameKey || item.dataKey || 'value']`, and a `` + carries NO `dataKey` — scatter's keys live on the two axes, not on + the mark. So the key collapsed to the literal `'value'`, missed a + config keyed by measure name, and the entry rendered its colour + swatch with NO TEXT beside it. + + What that looks like on screen is the whole card: an 8x8 dot in + `--chart-1` — the SAME colour as the marks — sitting under the + x-axis, which reads as a seventh data point plotted outside the + plot area. Measured on the showcase Chart Gallery in real + Chromium: swatch at cy 341 against a plot area ending at cy 295, + on a y scale of 4.835 px per unit, i.e. y = -9.5 — which is + exactly the "x≈40, y≈-10" the card reported as a stray point. + + The y DOMAIN was never the defect and is deliberately not touched: + the same run measured all six marks inside the plot area, and + mixed-sign and all-negative fixtures draw every mark inside too, + because recharts extends the domain to cover negatives. */} } + content={} {...(isMobile && { verticalAlign: "bottom", wrapperStyle: { fontSize: '11px', paddingTop: '8px' } })} /> {series.map((s: any, index: number) => { diff --git a/packages/plugin-charts/src/ChartContainerImpl.tsx b/packages/plugin-charts/src/ChartContainerImpl.tsx index 5f30d747c4..117306bae5 100644 --- a/packages/plugin-charts/src/ChartContainerImpl.tsx +++ b/packages/plugin-charts/src/ChartContainerImpl.tsx @@ -484,7 +484,21 @@ function ChartLegendContent({ }} /> )} - {itemConfig?.label} + {/* Fall back to the series name recharts itself put on the + legend item (objectui#7248). The config lookup above is an + OVERRIDE layer, so a miss must not erase the label: the + swatch renders unconditionally, and a swatch with no text is + an unidentifiable coloured dot — on a scatter it read as a + stray data point plotted outside the plot area. + + This is the general form of the pie bug the helper below + documents ("legend swatches with NO text at all"). That one + was closed by making one family's key resolve; this closes + the class, because `item.value` IS the `name` the mark + declared and is the last thing standing between a reader and + an anonymous dot. Charts whose config already resolves are + unaffected — only a currently-EMPTY label changes. */} + {itemConfig?.label ?? item.value} ) })}