diff --git a/.changeset/chartconfig-remaining-inert-props.md b/.changeset/chartconfig-remaining-inert-props.md new file mode 100644 index 000000000..de6ac9ff1 --- /dev/null +++ b/.changeset/chartconfig-remaining-inert-props.md @@ -0,0 +1,31 @@ +--- +"@object-ui/plugin-charts": minor +--- + +feat(charts): honor `ChartAxis.stepSize`, `ChartConfig.description` and `ChartConfig.height` (framework#3752) + +The tail of the declared-≠-delivered sweep from framework#3729 / #2880. Three +`ChartConfig` props reached the renderer and did nothing: + +- **`ChartAxis.stepSize`** — Recharts has no "a tick every N units" prop + (`tickCount` is a hint it may ignore, `interval` is for categorical axes), so + honoring a step means handing it the tick array outright. `ticksFor` builds it + from the axis's own `min`/`max` where declared and from the plotted values + otherwise, so a step works with or without a pinned domain. A data-derived max + rounds UP to the next step (otherwise the topmost bar sits above the last + gridline and the axis reads as truncated); an explicit `max` clamps instead, + since a tick outside a pinned domain would be drawn outside the plot. A step + that would produce more than 200 ticks is refused rather than rendered — that + is a wrong config, and drawing it would hang the page instead of surfacing the + mistake. +- **`ChartConfig.description`** — the accessibility description. A chart is a + picture to a screen reader; the container now carries `role="img"` + + `aria-label`. Without a description it stays an ordinary div, because + stamping `role="img"` on an *unlabelled* graphic is worse than leaving one a + screen reader can skip. +- **`ChartConfig.height`** — was read only by the legacy `ChartBarRenderer`, not + by the advanced path that draws every real chart. Now applied to the chart + container as an inline style, which beats its default `h-[350px]` class. + +`height` and `description` ride on the shared container props, so they apply to +all eight chart families rather than one branch. diff --git a/packages/plugin-charts/src/AdvancedChartImpl.specConfig.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.specConfig.test.tsx index 0867fdb10..b71a01ce4 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.specConfig.test.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.specConfig.test.tsx @@ -151,6 +151,36 @@ describe('AdvancedChartImpl — spec annotations', () => { }); }); +describe('AdvancedChartImpl — spec container props (#3752)', () => { + it('announces the accessibility description', () => { + const { container } = render(); + const el = container.querySelector('[data-slot="chart"]'); + expect(el?.getAttribute('role')).toBe('img'); + expect(el?.getAttribute('aria-label')).toBe('Invoice value by status'); + }); + + it('leaves the container unlabelled when no description is declared', () => { + // No description is not the same as an empty one — don't stamp role="img" + // on an unlabelled graphic, which is worse for a screen reader than a + // plain div it can skip. + const { container } = render(); + expect(container.querySelector('[data-slot="chart"]')?.getAttribute('role')).toBeNull(); + }); + + it('applies an explicit height over the container default', () => { + const { container } = render(); + expect((container.querySelector('[data-slot="chart"]') as HTMLElement)?.style.height).toBe('420px'); + }); + + it('lays y ticks at the declared stepSize', () => { + const { container } = render( + , + ); + const texts = Array.from(container.querySelectorAll('text')).map((t) => t.textContent ?? ''); + for (const tick of ['0', '40', '80', '120']) expect(texts).toContain(tick); + }); +}); + describe('AdvancedChartImpl — spec interaction', () => { it('adds the brush when interaction.brush is on', () => { const { container } = render(); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index e8b1f9d49..f338784e2 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -47,7 +47,7 @@ import { ChartConfig } from './ChartContainerImpl'; import { mapScatterClick, mapTreemapClick, mapSankeyClick } from './chartDrillEvents'; -import { formatterFor, domainFor, type NormalizedAxis, type NormalizedSeries } from './normalizeChartSchema'; +import { formatterFor, domainFor, ticksFor, type NormalizedAxis, type NormalizedSeries } from './normalizeChartSchema'; import { buildCategoryRank } from '@object-ui/core'; // Default color fallback for chart series @@ -160,6 +160,14 @@ export interface AdvancedChartImplProps { /** Spec `ChartConfig.title` / `.subtitle`, rendered above the plot. */ title?: string; subtitle?: string; + /** + * Spec `ChartConfig.description` — the accessibility description. A chart is + * a picture to a screen reader; without this it announces as an unlabelled + * graphic, so the container carries it as `role="img"` + `aria-label`. + */ + description?: string; + /** Spec `ChartConfig.height` — fixed plot height in pixels. */ + height?: number; /** Spec `ChartConfig.annotations` — reference lines / bands. */ annotations?: Array>; /** Spec `ChartConfig.interaction` — `tooltips`, `brush`. */ @@ -238,6 +246,8 @@ export default function AdvancedChartImpl({ showDataLabels, title, subtitle, + description, + height, annotations, interaction, }: AdvancedChartImplProps) { @@ -252,7 +262,15 @@ export default function AdvancedChartImpl({ // tell ChartContainer to skip its settle re-mount — avoids a needless 1-frame // reflow on the dashboard's first paint (#2756). Animated callers keep the // heal; this object is empty for them, leaving their markup unchanged. - const containerProps = isAnimationActive === false ? { disableSettleRemount: true } : {}; + // Everything every ChartContainer call site needs, so a new container-level + // spec prop lands on all eight chart families at once instead of one branch. + const containerProps = { + ...(isAnimationActive === false ? { disableSettleRemount: true as const } : {}), + // An explicit height beats the container's default `h-[350px]` class + // because an inline style wins over a utility class. + ...(height ? { style: { height } } : {}), + ...(description ? { role: 'img' as const, 'aria-label': description } : {}), + }; const [isMobile, setIsMobile] = React.useState(false); // Recharts' top-level onClick payload: { activeLabel, activePayload, ... } @@ -434,11 +452,32 @@ export default function AdvancedChartImpl({ [secondaryY?.format, formatYTick], ); - /** Recharts props derived from one spec y-axis (domain / scale / label). */ - const yAxisSpecProps = React.useCallback((axis: NormalizedAxis | undefined) => { + /** + * Every number plotted on one side of a dual axis (or on the only axis) — + * the range `stepSize` lays its ticks over. + */ + const axisValues = React.useCallback((side: 'left' | 'right') => { + const keys = series + .filter((s: any) => (hasDualAxis ? (s.yAxis === 'right' ? 'right' : 'left') === side : side === 'left')) + .map((s: any) => s.dataKey); + const out: number[] = []; + for (const row of data) { + for (const k of keys) { + const n = Number(row?.[k]); + if (Number.isFinite(n)) out.push(n); + } + } + return out; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [data, series, hasDualAxis]); + + /** Recharts props derived from one spec y-axis (domain / scale / ticks / label). */ + const yAxisSpecProps = React.useCallback((axis: NormalizedAxis | undefined, side: 'left' | 'right' = 'left') => { if (!axis) return {}; const domain = domainFor(axis); + const ticks = ticksFor(axis, axisValues(side)); return { + ...(ticks ? { ticks } : {}), ...(domain ? { domain } : {}), // `allowDataOverflow` is what makes an explicit domain actually clip // rather than being silently widened to fit the data. @@ -446,7 +485,7 @@ export default function AdvancedChartImpl({ ...(axis.logarithmic ? { scale: 'log' as const, domain: domain ?? ([1, 'auto'] as any) } : {}), ...(axis.title ? { label: { value: axis.title, angle: -90, position: 'insideLeft' as const } } : {}), }; - }, []); + }, [axisValues]); // `showGridLines` is per-axis in the spec; the renderer draws one grid, so // an explicit `false` on EITHER axis turns off that axis's lines. @@ -785,7 +824,7 @@ export default function AdvancedChartImpl({ - + {tooltipsEnabled ? } /> : null} {legendVisible ? ( ) : null} diff --git a/packages/plugin-charts/src/ChartRenderer.tsx b/packages/plugin-charts/src/ChartRenderer.tsx index e07ad4534..9b4fec4f3 100644 --- a/packages/plugin-charts/src/ChartRenderer.tsx +++ b/packages/plugin-charts/src/ChartRenderer.tsx @@ -61,6 +61,8 @@ export interface ChartRendererProps { showDataLabels?: boolean; title?: unknown; subtitle?: unknown; + description?: unknown; + height?: number; annotations?: Array>; interaction?: Record; /** An author `type` rescued from the SDUI envelope's discriminator @@ -160,6 +162,8 @@ export const ChartRenderer: React.FC = ({ schema, onChartCli showDataLabels={props.spec.showDataLabels} title={props.spec.title} subtitle={props.spec.subtitle} + description={props.spec.description} + height={props.spec.height ?? schema.height} annotations={props.spec.annotations} interaction={props.spec.interaction} /> diff --git a/packages/plugin-charts/src/normalizeChartSchema.test.ts b/packages/plugin-charts/src/normalizeChartSchema.test.ts index c7bcd5f0d..4bd249f2d 100644 --- a/packages/plugin-charts/src/normalizeChartSchema.test.ts +++ b/packages/plugin-charts/src/normalizeChartSchema.test.ts @@ -10,7 +10,7 @@ * (DashboardRenderer / ObjectView / the dataset path pass it explicitly). */ import { describe, it, expect } from 'vitest'; -import { normalizeChartSchema, formatterFor, domainFor } from './normalizeChartSchema'; +import { normalizeChartSchema, formatterFor, domainFor, ticksFor } from './normalizeChartSchema'; describe('normalizeChartSchema — spec shape', () => { it('resolves the ChartConfig axis + series shape', () => { @@ -140,6 +140,59 @@ describe('formatterFor', () => { }); }); +describe('normalizeChartSchema — container-level props (#3752)', () => { + it('carries description and height', () => { + const out = normalizeChartSchema({ description: 'Invoice value by status', height: 320 }); + expect(out.description).toBe('Invoice value by status'); + expect(out.height).toBe(320); + }); + + it('ignores a non-positive height rather than collapsing the chart', () => { + expect(normalizeChartSchema({ height: 0 }).height).toBeUndefined(); + expect(normalizeChartSchema({ height: -10 }).height).toBeUndefined(); + }); + + it('carries stepSize on an axis', () => { + expect(normalizeChartSchema({ yAxis: [{ field: 'total', stepSize: 25 }] }).yAxes?.[0].stepSize).toBe(25); + }); + + it('ignores a non-positive stepSize', () => { + expect(normalizeChartSchema({ yAxis: [{ field: 'total', stepSize: 0 }] }).yAxes?.[0].stepSize).toBeUndefined(); + }); +}); + +describe('ticksFor', () => { + it('lays ticks over the data range at the declared step', () => { + expect(ticksFor({ stepSize: 50 }, [10, 120])).toEqual([0, 50, 100, 150]); + }); + + it('honours an explicit domain over the data', () => { + expect(ticksFor({ min: 100, max: 300, stepSize: 100 }, [10, 20])).toEqual([100, 200, 300]); + }); + + it('reaches an explicit max the step would otherwise overshoot', () => { + expect(ticksFor({ min: 0, max: 25, stepSize: 10 }, [])).toEqual([0, 10, 20, 25]); + }); + + it('does not drift on a fractional step', () => { + expect(ticksFor({ min: 0, max: 0.5, stepSize: 0.1 }, [])).toEqual([0, 0.1, 0.2, 0.3, 0.4, 0.5]); + }); + + it('returns undefined with no stepSize — Recharts keeps its automatic ticks', () => { + expect(ticksFor({ min: 0, max: 100 }, [])).toBeUndefined(); + expect(ticksFor(undefined, [])).toBeUndefined(); + }); + + it('refuses an absurd tick count instead of hanging the page', () => { + // 100_000 / 0.5 ticks is a wrong config, not a chart. + expect(ticksFor({ stepSize: 0.5 }, [0, 100_000])).toBeUndefined(); + }); + + it('returns undefined when there is nothing to measure', () => { + expect(ticksFor({ stepSize: 10 }, [])).toBeUndefined(); + }); +}); + describe('domainFor', () => { it('pins both ends', () => { expect(domainFor({ min: 0, max: 100 })).toEqual([0, 100]); diff --git a/packages/plugin-charts/src/normalizeChartSchema.ts b/packages/plugin-charts/src/normalizeChartSchema.ts index 708f3b539..5bc47edee 100644 --- a/packages/plugin-charts/src/normalizeChartSchema.ts +++ b/packages/plugin-charts/src/normalizeChartSchema.ts @@ -88,6 +88,8 @@ export interface NormalizedAxis { format?: string; min?: number; max?: number; + /** Distance between ticks on this axis. */ + stepSize?: number; /** Default true, per the spec schema. */ showGridLines?: boolean; logarithmic?: boolean; @@ -123,6 +125,10 @@ export interface NormalizedChartSchema { showDataLabels?: boolean; title?: string; subtitle?: string; + /** Accessibility description — announced to screen readers. */ + description?: string; + /** Fixed plot height in pixels. */ + height?: number; annotations?: AnyRec[]; interaction?: AnyRec; } @@ -156,6 +162,8 @@ function normalizeAxis(raw: unknown): NormalizedAxis | undefined { if (min !== undefined) out.min = min; const max = num(raw.max); if (max !== undefined) out.max = max; + const stepSize = num(raw.stepSize); + if (stepSize !== undefined && stepSize > 0) out.stepSize = stepSize; if (typeof raw.showGridLines === 'boolean') out.showGridLines = raw.showGridLines; if (typeof raw.logarithmic === 'boolean') out.logarithmic = raw.logarithmic; const position = str(raw.position); @@ -270,6 +278,10 @@ export function normalizeChartSchema(schema: unknown): NormalizedChartSchema { if (title) out.title = title; const subtitle = label(schema.subtitle); if (subtitle) out.subtitle = subtitle; + const description = label(schema.description); + if (description) out.description = description; + const height = num(schema.height); + if (height !== undefined && height > 0) out.height = height; if (Array.isArray(schema.annotations) && schema.annotations.length) { out.annotations = schema.annotations.filter(isRec); } @@ -321,6 +333,59 @@ export function formatterFor(format: string | undefined): ((value: any) => strin }; } +/** + * Explicit tick positions for an axis that declares a `stepSize`, or + * `undefined` to keep Recharts' automatic ticks. + * + * Recharts has no "every N units" prop — `tickCount` is a hint it may ignore + * and `interval` is for categorical axes — so honoring `stepSize` means + * handing it the tick array outright. The range comes from the axis's own + * `min`/`max` where declared and from the plotted values otherwise, so a step + * works with or without a pinned domain. + * + * `values` should be every number plotted on this axis. An empty range, a + * non-finite one, or a step that would produce an absurd number of ticks + * (>`MAX_TICKS`) yields `undefined` — a 10,000-tick axis is a wrong config, + * and drawing it would hang the page rather than report the mistake. + */ +const MAX_TICKS = 200; + +export function ticksFor(axis: NormalizedAxis | undefined, values: number[]): number[] | undefined { + const step = axis?.stepSize; + if (!step || step <= 0) return undefined; + + const finite = values.filter((v) => Number.isFinite(v)); + const dataMin = finite.length ? Math.min(...finite) : undefined; + const dataMax = finite.length ? Math.max(...finite) : undefined; + // A value axis conventionally starts at zero unless told otherwise, which is + // also what Recharts' own auto domain does for bars/areas. + const lo = axis.min ?? Math.min(0, dataMin ?? 0); + const hi = axis.max ?? dataMax; + if (hi === undefined || !Number.isFinite(lo) || !Number.isFinite(hi) || hi < lo) return undefined; + + const start = Math.floor(lo / step) * step; + // An explicit `max` pins the domain, so the last tick must not overshoot it + // (Recharts would place a tick outside the plot). A data-derived max is not + // pinned, so round UP to the next step — otherwise the topmost value sits + // above the last gridline and the axis reads as truncated. + const end = axis.max !== undefined ? axis.max : Math.ceil(hi / step) * step; + // `(end - start) / step` is exact in decimal but not in binary: 0.5 / 0.1 is + // 5.000000000000001 one way and 4.999999999999999 the other, and a bare + // floor() drops a whole tick in the second case. + const count = Math.floor((end - start) / step + 1e-9) + 1; + if (count < 1 || count > MAX_TICKS) return undefined; + + const out: number[] = []; + for (let i = 0; i < count; i++) { + // Re-derive from the index rather than accumulating, so a fractional step + // (0.1) does not drift into 0.30000000000000004 territory across the axis. + out.push(Number((start + i * step).toPrecision(12))); + } + // Make sure an explicit max is actually reachable as a tick. + if (axis.max !== undefined && out[out.length - 1] < axis.max) out.push(axis.max); + return out; +} + /** * Recharts `domain` for an axis, or `undefined` to keep the auto domain. * A half-open range (only `min`, only `max`) pins that end and leaves the