Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .changeset/chartconfig-remaining-inert-props.md
Original file line number Diff line number Diff line change
@@ -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.
30 changes: 30 additions & 0 deletions packages/plugin-charts/src/AdvancedChartImpl.specConfig.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,36 @@ describe('AdvancedChartImpl — spec annotations', () => {
});
});

describe('AdvancedChartImpl — spec container props (#3752)', () => {
it('announces the accessibility description', () => {
const { container } = render(<AdvancedChartImpl {...base} description="Invoice value by status" />);
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(<AdvancedChartImpl {...base} />);
expect(container.querySelector('[data-slot="chart"]')?.getAttribute('role')).toBeNull();
});

it('applies an explicit height over the container default', () => {
const { container } = render(<AdvancedChartImpl {...base} height={420} />);
expect((container.querySelector('[data-slot="chart"]') as HTMLElement)?.style.height).toBe('420px');
});

it('lays y ticks at the declared stepSize', () => {
const { container } = render(
<AdvancedChartImpl {...base} yAxes={[{ field: 'total', min: 0, max: 120, stepSize: 40 }]} />,
);
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(<AdvancedChartImpl {...base} interaction={{ brush: true }} />);
Expand Down
53 changes: 46 additions & 7 deletions packages/plugin-charts/src/AdvancedChartImpl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Record<string, any>>;
/** Spec `ChartConfig.interaction` — `tooltips`, `brush`. */
Expand Down Expand Up @@ -238,6 +246,8 @@ export default function AdvancedChartImpl({
showDataLabels,
title,
subtitle,
description,
height,
annotations,
interaction,
}: AdvancedChartImplProps) {
Expand All @@ -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, ... }
Expand Down Expand Up @@ -434,19 +452,40 @@ 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.
...(domain ? { allowDataOverflow: true } : {}),
...(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.
Expand Down Expand Up @@ -785,7 +824,7 @@ export default function AdvancedChartImpl({
<CartesianGrid {...gridProps} />
<XAxis dataKey={xAxisKey} {...xAxisCommonProps} />
<YAxis yAxisId="left" tickLine={false} axisLine={false} tickFormatter={yTickFormatter} width={48} {...yAxisSpecProps(primaryY)} />
<YAxis yAxisId="right" orientation="right" tickLine={false} axisLine={false} tickFormatter={y2TickFormatter} width={48} {...yAxisSpecProps(secondaryY)} />
<YAxis yAxisId="right" orientation="right" tickLine={false} axisLine={false} tickFormatter={y2TickFormatter} width={48} {...yAxisSpecProps(secondaryY, 'right')} />
{tooltipsEnabled ? <ChartTooltip content={<ChartTooltipContent />} /> : null}
{legendVisible ? (
<ChartLegend
Expand Down Expand Up @@ -902,7 +941,7 @@ export default function AdvancedChartImpl({
axisLine={false}
tickFormatter={y2TickFormatter}
width={48}
{...yAxisSpecProps(secondaryY)}
{...yAxisSpecProps(secondaryY, 'right')}
/>
) : null}
</>
Expand Down
4 changes: 4 additions & 0 deletions packages/plugin-charts/src/ChartRenderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ export interface ChartRendererProps {
showDataLabels?: boolean;
title?: unknown;
subtitle?: unknown;
description?: unknown;
height?: number;
annotations?: Array<Record<string, any>>;
interaction?: Record<string, any>;
/** An author `type` rescued from the SDUI envelope's discriminator
Expand Down Expand Up @@ -160,6 +162,8 @@ export const ChartRenderer: React.FC<ChartRendererProps> = ({ 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}
/>
Expand Down
55 changes: 54 additions & 1 deletion packages/plugin-charts/src/normalizeChartSchema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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]);
Expand Down
65 changes: 65 additions & 0 deletions packages/plugin-charts/src/normalizeChartSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down
Loading