Skip to content

Commit 9b53d72

Browse files
authored
feat(charts): ObjectChart honors the spec ChartConfig author shape (#2880) (#2883)
ChartConfigSchema is the chart protocol, but the renderer only ever read a Recharts-flavoured internal shape — chartType, xAxisKey, series[].dataKey. Everything an author wrote in the SPEC shape reached the renderer and was silently dropped, which is what ADR-0078 forbids. S1 — normalizeChartSchema translates the author shape into the internal pipeline contract in ONE place rather than scattering `??` fallbacks through the render tree: type→chartType, xAxis:{field}→xAxisKey, series:[{name}]→[{dataKey}], the report surface's bare-string axes, and yAxis:[{field}] alone as the plotted series. Internal props win, so DashboardRenderer/ObjectView/the dataset path are byte-for-byte unaffected. The `type` collision: on any surface that flattens chart config into a props bag, `type` is already the SDUI envelope's component discriminator — an author writing type="bar" replaced object-chart and the block stopped resolving. The react-page wrapper now keeps both: the discriminator wins the `type` slot and the author's value is preserved beside it as specType. S2 — ChartAxis.format drives the tick formatter (Intl.NumberFormat, no new dependency), min/max pin the domain, logarithmic swaps the scale, title labels the axis, showGridLines is honored. A second yAxis entry (or position:'right') turns on the secondary axis that series[].yAxis binds to, beating the family-derived guess in combo charts. showLegend is honored and title/subtitle render above the plot. S3 — series[].stack passes through as Recharts' stackId; annotations render as ReferenceLine/ReferenceArea honoring axis/value/endValue/color/style/label; interaction.tooltips:false suppresses the hover card, interaction.brush:true adds the range selector, showDataLabels prints values on the marks. interaction.zoom has no Recharts primitive behind it and stays unimplemented rather than faked.
1 parent 7d46648 commit 9b53d72

7 files changed

Lines changed: 1040 additions & 41 deletions

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@object-ui/plugin-charts": minor
3+
"@object-ui/components": patch
4+
---
5+
6+
feat(charts): ObjectChart honors the spec `ChartConfig` author shape (objectui#2880 / framework#3729)
7+
8+
`ChartConfigSchema` is the chart protocol, but the renderer only ever read a
9+
Recharts-flavoured internal shape — `chartType`, `xAxisKey`, `series[].dataKey`.
10+
Everything an author wrote in the SPEC shape reached the renderer and was
11+
silently dropped, which is exactly what ADR-0078 forbids. framework#3725
12+
documented the gap by trimming the published contract down to the props that
13+
actually worked; this closes it the other way round.
14+
15+
**S1 — one normalization boundary.** `normalizeChartSchema` translates the
16+
author shape into the internal pipeline contract in a single place, rather than
17+
scattering `??` fallbacks through the render tree (framework PD #12: one
18+
translation is a contract mapping, N fallbacks are a second dialect):
19+
20+
- `type``chartType`, `xAxis: { field }``xAxisKey`, `series: [{ name }]`
21+
`series: [{ dataKey }]`
22+
- the report surface's bare-string `xAxis`/`yAxis` resolve too
23+
- `yAxis: [{ field }]` alone plots, with no `series` declared
24+
- **internal props win**, so `DashboardRenderer`, `ObjectView` and the dataset
25+
path are byte-for-byte unaffected — there is no migration
26+
27+
**The `type` collision.** `ChartConfig.type` is the chart family, but on any
28+
surface that flattens chart config into a props bag `type` is already the SDUI
29+
envelope's component discriminator. Spreading props last let an author's
30+
`type="bar"` replace `object-chart` so the block stopped resolving; stamping the
31+
discriminator last ate the author's value instead. The react-page wrapper now
32+
keeps both: the discriminator wins the `type` slot and the author's value is
33+
preserved beside it as `specType`, which the normalizer reads back.
34+
35+
**S2 — axis presentation.** `ChartAxis.format` drives the tick formatter (via
36+
`Intl.NumberFormat`, no new dependency), `min`/`max` pin the domain,
37+
`logarithmic` swaps the scale, `title` labels the axis, and `showGridLines` is
38+
honored. A second `yAxis` entry (or `position: 'right'`) turns on the secondary
39+
axis that `series[].yAxis` binds to — in combo charts an explicit binding now
40+
beats the family-derived bar→left/line→right guess. `showLegend` is honored,
41+
and `title`/`subtitle` render above the plot instead of only titling the
42+
drill-down drawer.
43+
44+
**S3 — `series[].stack`, `annotations`, `interaction`.** Stacking passes the
45+
author's group name through as Recharts' `stackId`. Annotations render as
46+
`ReferenceLine` (`type: 'line'`) / `ReferenceArea` (`type: 'region'`) with the
47+
declared axis, colour, style and label. `interaction.tooltips: false` suppresses
48+
the hover card and `interaction.brush: true` adds the range selector;
49+
`showDataLabels` prints values on the marks. `interaction.zoom` has no Recharts
50+
primitive behind it and is deliberately still unimplemented rather than faked.

packages/components/src/renderers/layout/react-page.tsx

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,21 @@ function buildComponentScope(dataSource: unknown): Record<string, React.Componen
5959
const name = toPascal(tag);
6060
if (seen.has(name)) continue;
6161
seen.add(name);
62-
const Wrapper: React.FC<any> = ({ children: _children, ...props }) =>
63-
React.createElement(SchemaRenderer as any, { schema: { type: tag, dataSource, ...props } });
62+
// `type` is the SDUI envelope's component discriminator, but it is ALSO a
63+
// legitimate prop name in a block's own spec schema — `ChartConfig.type` is
64+
// the chart family. Flattening props into the schema bag collides the two:
65+
// spreading last let an author's `type="bar"` replace `object-chart` and
66+
// the block stopped resolving; stamping the discriminator last silently ate
67+
// the author's value instead. Neither is acceptable (ADR-0078), so the
68+
// discriminator wins the `type` slot and the author's value is preserved
69+
// beside it under `specType` for the block to read
70+
// (objectui#2880 / framework#3729).
71+
const Wrapper: React.FC<any> = ({ children: _children, ...props }) => {
72+
const specType = typeof props.type === 'string' && props.type !== tag ? props.type : undefined;
73+
return React.createElement(SchemaRenderer as any, {
74+
schema: { dataSource, ...props, ...(specType ? { specType } : {}), type: tag },
75+
});
76+
};
6477
Wrapper.displayName = name;
6578
scope[name] = Wrapper;
6679
}
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* framework#3729 / objectui#2880 — the spec's `ChartConfig` presentation props
6+
* are honored, not silently dropped.
7+
*
8+
* `showLegend`, `title`/`subtitle`, `yAxis[].min/max/format/logarithmic`,
9+
* `series[].stack`, `series[].yAxis`, `annotations` and `interaction` were all
10+
* declared by `ChartConfigSchema`, reached this renderer, and did nothing —
11+
* exactly the silently-inert metadata ADR-0078 forbids. These pin the wiring
12+
* at the DOM/prop level so a future refactor cannot quietly drop them again.
13+
*/
14+
15+
import React from 'react';
16+
import { describe, it, expect, vi, afterEach } from 'vitest';
17+
import { render, cleanup, screen } from '@testing-library/react';
18+
19+
// Recharts' ResponsiveContainer measures via ResizeObserver, which reports 0×0
20+
// under the headless DOM, so nothing paints. Fix its size.
21+
vi.mock('recharts', async () => {
22+
const actual = await vi.importActual<any>('recharts');
23+
return {
24+
...actual,
25+
ResponsiveContainer: ({ children }: any) =>
26+
React.cloneElement(children, { width: 480, height: 320 }),
27+
};
28+
});
29+
30+
import AdvancedChartImpl from './AdvancedChartImpl';
31+
32+
afterEach(cleanup);
33+
34+
const DATA = [
35+
{ status: 'open', total: 120, rate: 0.4 },
36+
{ status: 'paid', total: 80, rate: 0.9 },
37+
];
38+
39+
const base = {
40+
chartType: 'bar' as const,
41+
data: DATA,
42+
xAxisKey: 'status',
43+
series: [{ dataKey: 'total' }],
44+
isAnimationActive: false,
45+
};
46+
47+
describe('AdvancedChartImpl — spec ChartConfig chrome', () => {
48+
it('renders title and subtitle above the plot', () => {
49+
render(<AdvancedChartImpl {...base} title="Invoice value" subtitle="by status" />);
50+
expect(screen.getByText('Invoice value')).toBeTruthy();
51+
expect(screen.getByText('by status')).toBeTruthy();
52+
});
53+
54+
it('adds no wrapper chrome when neither is set', () => {
55+
const { container } = render(<AdvancedChartImpl {...base} />);
56+
// ChartFrame is a passthrough with no title/subtitle, so the container's
57+
// first child is still the chart itself.
58+
expect(container.querySelector('.text-muted-foreground')).toBeNull();
59+
});
60+
61+
it('honors showLegend: false', () => {
62+
const { container: withLegend } = render(<AdvancedChartImpl {...base} />);
63+
const legendCount = withLegend.querySelectorAll('.recharts-legend-wrapper').length;
64+
cleanup();
65+
const { container: without } = render(<AdvancedChartImpl {...base} showLegend={false} />);
66+
expect(without.querySelectorAll('.recharts-legend-wrapper').length).toBeLessThan(legendCount);
67+
});
68+
});
69+
70+
describe('AdvancedChartImpl — spec ChartAxis', () => {
71+
it('formats y ticks with the declared format instead of the compact default', () => {
72+
const { container } = render(
73+
<AdvancedChartImpl {...base} yAxes={[{ field: 'total', format: '$0,0.00' }]} />,
74+
);
75+
// Recharts 3 draws tick labels in their own z-index layer rather than
76+
// inside the axis group, so read every <text> and look for the currency
77+
// symbol — the compact default would render a bare "120".
78+
const texts = Array.from(container.querySelectorAll('text')).map((t) => t.textContent ?? '');
79+
expect(texts.some((t) => t.includes('$'))).toBe(true);
80+
});
81+
82+
it('renders an axis title', () => {
83+
render(<AdvancedChartImpl {...base} yAxes={[{ field: 'total', title: 'Revenue' }]} />);
84+
expect(screen.getByText('Revenue')).toBeTruthy();
85+
});
86+
87+
it('draws a second y-axis when two are declared', () => {
88+
const { container: single } = render(<AdvancedChartImpl {...base} />);
89+
const singleCount = single.querySelectorAll('.recharts-yAxis').length;
90+
cleanup();
91+
const { container: dual } = render(
92+
<AdvancedChartImpl
93+
{...base}
94+
series={[{ dataKey: 'total' }, { dataKey: 'rate', yAxis: 'right' }]}
95+
yAxes={[{ field: 'total' }, { field: 'rate', position: 'right' }]}
96+
/>,
97+
);
98+
expect(dual.querySelectorAll('.recharts-yAxis').length).toBeGreaterThan(singleCount);
99+
});
100+
});
101+
102+
describe('AdvancedChartImpl — spec series.stack', () => {
103+
/** X offsets of every drawn bar (Recharts 3 draws bars as <path>). */
104+
const barXs = (container: HTMLElement) =>
105+
Array.from(container.querySelectorAll('.recharts-bar-rectangle path')).map((p) => p.getAttribute('x'));
106+
107+
it('stacks series sharing a stack group', () => {
108+
// Recharts stacks by `stackId`. Stacked: both series of a category share
109+
// one column band → 2 distinct x for 2 categories × 2 series. Grouped:
110+
// the bars sit side by side → 4 distinct x.
111+
const { container: stacked } = render(
112+
<AdvancedChartImpl
113+
{...base}
114+
series={[
115+
{ dataKey: 'total', stack: 'g' },
116+
{ dataKey: 'rate', stack: 'g' },
117+
]}
118+
/>,
119+
);
120+
const stackedXs = barXs(stacked);
121+
expect(stackedXs.length).toBe(4);
122+
expect(new Set(stackedXs).size).toBe(2);
123+
cleanup();
124+
125+
const { container: grouped } = render(
126+
<AdvancedChartImpl {...base} series={[{ dataKey: 'total' }, { dataKey: 'rate' }]} />,
127+
);
128+
expect(new Set(barXs(grouped)).size).toBe(4);
129+
});
130+
});
131+
132+
describe('AdvancedChartImpl — spec annotations', () => {
133+
it('draws a reference line for a line annotation', () => {
134+
const { container } = render(
135+
<AdvancedChartImpl {...base} annotations={[{ type: 'line', axis: 'y', value: 100, label: 'Target' }]} />,
136+
);
137+
expect(container.querySelectorAll('.recharts-reference-line').length).toBeGreaterThan(0);
138+
expect(screen.getByText('Target')).toBeTruthy();
139+
});
140+
141+
it('draws a reference area for a region annotation', () => {
142+
const { container } = render(
143+
<AdvancedChartImpl {...base} annotations={[{ type: 'region', axis: 'y', value: 50, endValue: 100 }]} />,
144+
);
145+
expect(container.querySelectorAll('.recharts-reference-area').length).toBeGreaterThan(0);
146+
});
147+
148+
it('draws nothing extra when no annotation is declared', () => {
149+
const { container } = render(<AdvancedChartImpl {...base} />);
150+
expect(container.querySelectorAll('.recharts-reference-line').length).toBe(0);
151+
});
152+
});
153+
154+
describe('AdvancedChartImpl — spec interaction', () => {
155+
it('adds the brush when interaction.brush is on', () => {
156+
const { container } = render(<AdvancedChartImpl {...base} interaction={{ brush: true }} />);
157+
expect(container.querySelectorAll('.recharts-brush').length).toBeGreaterThan(0);
158+
});
159+
160+
it('omits the brush by default', () => {
161+
const { container } = render(<AdvancedChartImpl {...base} />);
162+
expect(container.querySelectorAll('.recharts-brush').length).toBe(0);
163+
});
164+
});

0 commit comments

Comments
 (0)