Charts: Extract shared ChartLayout component for chart+legend layout by adamwoodnz · Pull Request #47554 · Automattic/jetpack

adamwoodnz · 2026-03-11T23:58:06Z

Introduces a ChartLayout component that encapsulates the shared Stack + legend positioning pattern duplicated across all 5 chart types. ChartLayout supports a render prop pattern with built-in useElementSize measurement, eliminating duplicated resize logic from individual charts.

Fixes CHARTS-181: Extract shared ChartLayout component for chart + legend layout pattern

Proposed changes

Add ChartLayout component wrapping Stack direction="column" with legend slot rendering (top/bottom), visibility management, and optional render prop measurement via useElementSize
Migrate all 5 charts (LineChart, BarChart, PieChart, PieSemiCircleChart, LeaderboardChart) to use ChartLayout, removing duplicated Stack + legend positioning code from each
Add render prop support (children: ReactNode | (measurements => ReactNode)) so charts that need measurement (line, bar, pie) get contentWidth/contentHeight from a shared content wrapper div
Consolidate to a single SingleChartContext.Provider per chart — previously some charts had redundant inner providers
Extract shared CSS rules (flex: 1; min-height: 0; min-width: 0) into chart-layout.module.scss, removing duplicated svg-wrapper blocks from 4 chart SCSS files
Use Stack component for pie chart SVG centering instead of custom flex CSS (per project guidelines)
Remove hardcoded size props from pie chart stories that were masking responsive sizing behavior
Add 12 tests for ChartLayout covering legend positioning, render prop measurement, visibility, ref forwarding, and composition API slot rendering

Other information

Generate changelog entries for this PR (using AI).

Related product discussion/links

CHARTS-181

Does this pull request change what data or activity we track or use?

No

Testing instructions

Run the chart tests to verify all pass:
```
pnpm run --filter=@automattic/charts test
```
Expected: 792 tests pass across 42 suites.
Start Storybook and verify each chart type renders correctly:
```
pnpm run --filter=@automattic/charts storybook
```
- Line Chart: Check default, with legend (top/bottom), and composition legend stories
- Bar Chart: Check default, with legend, and responsive resizing
- Pie Chart: Check default (no size prop — should fill container), with composition legend, and donut variants
- Semi-circle Chart: Check default and with legend stories
- Leaderboard Chart: Check default and empty state
For pie charts specifically, verify responsive sizing:
- Drag the Storybook container resize handle — the chart should grow and shrink with the container
- Add a <PieChart.Legend> composition child — chart should still resize correctly
- Toggle legendPosition between top/bottom — layout should adjust without breaking sizing
Verify legend positioning works for all charts:
- Set legendPosition: 'top' and legendPosition: 'bottom' in controls
- Legend should render above or below the chart content respectively

Introduces a ChartLayout component that encapsulates the shared Stack + legend positioning pattern used by all chart components. Includes comprehensive tests covering legend positioning, visibility management, ref forwarding, and composition API slot rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-03-11T23:58:59Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the charts-181-extract-shared-chartlayout-component-for-chart-legend-layout branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack charts-181-extract-shared-chartlayout-component-for-chart-legend-layout

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2026-03-11T23:59:13Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jp-launch-control · 2026-03-12T00:16:31Z

Code Coverage Summary

Coverage changed in 2 files.

File	Coverage	Δ%	Δ Uncovered
projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx	90/96 (93.75%)	0.07%	0 💚
projects/js-packages/charts/src/charts/line-chart/line-chart.tsx	146/151 (96.69%)	0.02%	0 💚

2 files are newly checked for coverage.

File	Coverage
projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx	11/11 (100.00%) 💚
projects/js-packages/charts/src/charts/private/chart-layout/index.ts	0/0 (—%) 🤷

Full summary · PHP report · JS report

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ChartLayout now accepts children as either ReactNode or a render prop receiving ContentMeasurements (contentRef, contentWidth, contentHeight, isMeasured). This allows charts to consolidate their useElementSize calls into the shared layout component. Adds waitForMeasurement prop that hides the layout until content is measured. The legacy isWaitingForMeasurement prop is preserved as deprecated for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Move SingleChartContext.Provider to wrap ChartLayout (outside) so composition API legends in legend slots can access the chart context. Keep inner provider inside the render prop for measured dimensions. Remove waitForMeasurement since PieChart uses fallback dimensions (300x300) when unmeasured and doesn't need to hide. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…t render prop Remove individual useElementSize calls from each chart and use ChartLayout's render prop to receive content measurements instead. Pattern: outer SingleChartContext.Provider with fallback dimensions (for composition API legends in layout slots), inner provider inside render prop with measured dimensions (for chart internals). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Replace the expectDomOrder helper with direct compareDocumentPosition assertions. The helper required eslint-disable no-bitwise for the masking check; inline expect(...).toBe(Node.DOCUMENT_POSITION_FOLLOWING) is clearer and avoids the suppression. Made-with: Cursor

ChartLayout now wraps render prop children in a content div with shared flex styles (flex: 1; min-height: 0; min-width: 0), removing duplicated svg-wrapper rules from line-chart, bar-chart, pie-chart, and pie-semi-circle-chart SCSS modules. Charts no longer need to attach contentRef — ChartLayout handles measurement internally via the content wrapper div. Replace custom flex centering CSS in pie-chart and pie-semi-circle-chart svg-wrappers with Stack components using align/justify props, per project guidelines preferring layout primitives over ad-hoc flex rules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…arts The pie-chart class had `display: flex; flex-direction: column; align-items: center` which duplicated what the Stack component already provides. The `align-items: center` also prevented the chart-layout content area from stretching to full width. Similarly, the centering Stack inside both pie charts had inline `style={{ width: '100%', height: '100%' }}` that is unnecessary now that ChartLayout's content wrapper handles flex sizing. Also removes hardcoded `size` props from two pie chart stories that were masking the responsive sizing behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Superpowers directories are now ignored via .git/info/exclude. Removing the previously tracked plan file so it stays local. Made-with: Cursor

adamwoodnz · 2026-03-12T23:09:04Z

@claude again please

claude · 2026-03-12T23:09:27Z

Claude finished @adamwoodnz's task in 1m 39s —— View job

Code Review

All issues from the previous two reviews have been cleanly resolved. Here's the current state:

Resolved ✅

waitForMeasurement footgun — Eliminated entirely. ChartLayout auto-detects render-prop vs ReactNode children. The visibility guard is clean: only applies when isRenderProp && !isMeasured. Plain ReactNode children are never hidden. No footgun possible.

BarChart SingleChartContext.chartHeight — Fixed correctly. measuredChartHeight state + useCallback-wrapped handleContentHeightChange passed as onContentHeightChange. The provider now receives the actual drawable area height. Matches LineChart's pattern exactly.

PieChart/PieSemiCircleChart zero-dimensions context — Cleanly resolved by making chartWidth/chartHeight optional on ChartInstanceContextValue. The provider now just passes { chartId } for pie charts, which is honest rather than passing misleading zeros.

Pie chart centering fill behavior — Fixed via CSS class (pie-chart__centering / pie-semi-circle-chart__centering) with width: 100%; height: 100%. Using the SCSS module rather than inline styles is the cleaner approach given project conventions.

Tests — All 12 tests are well-targeted. The render-prop measurement test, visibility-hidden test, and plain-ReactNode no-hiding test cover the critical behaviors introduced by ChartLayout.

Remaining minor observations (non-blocking)

isMeasured checks only height — The author's reasoning holds: a zero-height chart isn't a valid state, and contentWidth === 0 with contentHeight > 0 isn't a realistic scenario given both are measured from the same ResizeObserver target. The current behavior is sufficient.

legendChildren required — Keeping it required is defensible since all callers go through useChartChildren. The friction tradeoff is acceptable.

Two ChartLayout instances in LeaderboardChart — The empty/loading branch and the data branch each own a SingleChartContext.Provider. This is structurally clean — each branch is independent and this avoids conditional provider rendering.

What's working well

The refactor is solid end-to-end. The render-prop measurement pattern via ChartLayout cleanly replaces duplicated Stack + visibility + resize boilerplate across all five charts. The onContentHeightChange → measuredChartHeight → SingleChartContext.chartHeight chain correctly propagates the drawable area height to consumers. The chart-layout__content shared SCSS (flex: 1; min-height: 0; min-width: 0) consolidates a real maintenance hazard. The forwardRef for pie chart portal tooltip bounds is a correct integration.

This looks ready to merge. 🟢

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

projects/js-packages/charts/tests/setup-element-size-mock.js

projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx

projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx

projects/js-packages/charts/tests/setup-element-size-mock.js

The setup-element-size-mock.js introduced in e242b7d had three flaws: 1. Accessed Element.prototype unconditionally, crashing the SSR test (jest-environment node) with "Element is not defined" 2. Returned hardcoded 800×400 for ALL zero-sized elements, overriding explicit dimensions (e.g. Sparkline at 100×40 got height 400) 3. The visx tooltip mock removed in 92201ce was NOT redundant — react-use-measure has a mounted.current guard that prevents state updates when ResizeObserver fires synchronously during the ref callback, leaving containerBounds at {0,0,...} and suppressing all tooltips Fixes: - Guard Element access with typeof check for SSR compatibility - Walk up DOM tree for ancestor inline style dimensions instead of hardcoded fallback, so charts with explicit sizes measure correctly - Restore visx tooltip mock with updated documentation explaining the react-use-measure timing issue - Override getBoundingClientRect locally in chart-layout visibility test to simulate unmeasured state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

When children is a plain ReactNode, the content measurement ref is never attached, so contentHeight stays 0. Firing onContentHeightChange(0) in that case is misleading for callers who wire up the callback without realizing it only produces meaningful values in render-prop mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx

projects/js-packages/charts/tests/setup-element-size-mock.js

Ensures the global getBoundingClientRect mock is restored even if an assertion throws, preventing leak into subsequent tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

dognose24

I got test errors all about src/charts/line-chart/test/line-chart.test.tsx for pnpm run --filter=@automattic/charts test. Did I miss anything?

dognose24 · 2026-03-16T08:54:01Z

projects/js-packages/charts/src/charts/private/single-chart-context/single-chart-context.tsx

 	chartRef?: React.RefObject< ChartInstanceRef >;
-	chartWidth: number;
-	chartHeight: number;
+	chartWidth?: number;


Making chartWidth/chartHeight optional changes the contract for context consumers. line-chart-annotations-overlay.tsx reads both values and passes them directly to SVG width/height attributes and DataContext — undefined would break the overlay.

Would it be better to add an early return guard in the overlay, consistent with the existing ones?

if ( ! chartWidth || ! chartHeight ) { return null; }

Good catch, fixed in 5cb7e25

kangzj · 2026-03-16T20:37:48Z

projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx

+						{ renderedChildren }
+					</div>
+				) : (
+					renderedChildren


Would it make sense to align the element structure here with the case where children is a function? Are there any special considerations I might have missed?

The wrapping <div ref={contentRef}> exists solely to attach the useElementSize ref for measurement — render-prop consumers need measured contentWidth/contentHeight to size their charts. Non-render-prop children (e.g. leaderboard) don't need measurement, so the extra wrapper would just add an unnecessary DOM node and could interfere with layout/styling (the wrapper has flex: 1; min-height: 0; min-width: 0 rules that assume it's sizing chart content).

kangzj · 2026-03-16T20:40:27Z

projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx

+				legendElement={ legendElement }
+				legendChildren={ legendChildren }


What happens if both passed?

Both will be rendered. This can be tested in storybook using the showLegend control on one of the Composition Legend stories.

Seems odd but I think it's valid configuration. Maybe a consumer could need two; tall chart, separate items, ...?

Not sure if I see the point in blocking it.

kangzj

Smoked tested all work well. Leave two comments no blockers. Thanks for working on this. Looks so much cleaner 👍

adamwoodnz · 2026-03-16T21:45:30Z

I got test errors all about src/charts/line-chart/test/line-chart.test.tsx for pnpm run --filter=@automattic/charts test. Did I miss anything?

I have seen these tick tests fail locally but they are passing for me currently, and in CI 🤷

…erlay Updated the early return condition to include checks for chartWidth and chartHeight, ensuring that the component only renders when all necessary dimensions are available. This prevents potential rendering issues when the chart is not properly sized.

Composition legend stories for bar, pie, donut, and semi-circle were cherry-picking individual props instead of spreading args. This meant Storybook controls like showLegend had no effect, preventing dual legend rendering (showLegend + composition <Chart.Legend>) that works in line chart and leaderboard stories. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

adamwoodnz self-assigned this Mar 11, 2026

Charts: Export ChartLayoutProps type from barrel

e621a42

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions bot added [JS Package] Charts [Status] In Progress [Tests] Includes Tests RNA labels Mar 11, 2026

github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 11, 2026

adamwoodnz and others added 6 commits March 12, 2026 13:01

Charts: Migrate LineChart to use ChartLayout

38db8be

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Charts: Migrate BarChart to use ChartLayout

8b757a7

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Charts: Migrate PieChart to use ChartLayout

6125f8b

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Charts: Migrate PieSemiCircleChart to use ChartLayout

db08da8

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Charts: Migrate LeaderboardChart to use ChartLayout

8bda08e

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Charts: Restore responsive/loading classes in LeaderboardChart empty …

51d2eab

…state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

adamwoodnz and others added 5 commits March 12, 2026 14:31

Charts: PieChart uses ChartLayout render prop for measurement

504ea45

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add changelog entries.

216809c

adamwoodnz force-pushed the charts-181-extract-shared-chartlayout-component-for-chart-legend-layout branch from 564414e to 216809c Compare March 12, 2026 02:50

github-actions bot added the Docs label Mar 12, 2026

adamwoodnz and others added 2 commits March 12, 2026 16:13

adamwoodnz force-pushed the charts-181-extract-shared-chartlayout-component-for-chart-legend-layout branch from 3e7180a to 9496b45 Compare March 12, 2026 04:02

chore(charts): Remove superpowers plan from tracked files

4710952

Superpowers directories are now ignored via .git/info/exclude. Removing the previously tracked plan file so it stays local. Made-with: Cursor

adamwoodnz changed the title ~~Charts: Add ChartLayout component for shared chart+legend layout~~ Charts: Extract shared ChartLayout component for chart+legend layout Mar 12, 2026

adamwoodnz requested a review from Copilot March 12, 2026 23:08

Copilot started reviewing on behalf of adamwoodnz March 12, 2026 23:09 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

adamwoodnz and others added 2 commits March 13, 2026 12:38

adamwoodnz requested a review from Copilot March 12, 2026 23:57

Copilot started reviewing on behalf of adamwoodnz March 12, 2026 23:58 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

test(charts): Wrap mock override in try/finally

595bbae

Ensures the global getBoundingClientRect mock is restored even if an assertion throws, preventing leak into subsequent tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

adamwoodnz requested a review from Copilot March 13, 2026 01:03

Copilot started reviewing on behalf of adamwoodnz March 13, 2026 01:04 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx Outdated Show resolved Hide resolved

projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx Show resolved Hide resolved

Only call onContentHeightChange if measured

3539809

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

adamwoodnz force-pushed the charts-181-extract-shared-chartlayout-component-for-chart-legend-layout branch 2 times, most recently from c082a9e to 3539809 Compare March 16, 2026 02:11

kangzj requested a review from a team March 16, 2026 02:51

dognose24 reviewed Mar 16, 2026

View reviewed changes

kangzj reviewed Mar 16, 2026

View reviewed changes

kangzj previously approved these changes Mar 16, 2026

View reviewed changes

adamwoodnz dismissed kangzj’s stale review via 5cb7e25 March 16, 2026 21:49

adamwoodnz merged commit 75bb73f into trunk Mar 17, 2026
89 checks passed

adamwoodnz deleted the charts-181-extract-shared-chartlayout-component-for-chart-legend-layout branch March 17, 2026 00:03

github-actions bot removed the [Status] In Progress label Mar 17, 2026

adamwoodnz mentioned this pull request Mar 17, 2026

Charts: Add TimeSeriesForecastChart component #46844

Open

3 tasks

		legendElement={ legendElement }
		legendChildren={ legendChildren }

Conversation

adamwoodnz commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Other information

Related product discussion/links

Does this pull request change what data or activity we track or use?

Testing instructions

Uh oh!

github-actions bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jp-launch-control bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Uh oh!

adamwoodnz commented Mar 12, 2026

Uh oh!

claude bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Resolved ✅

Remaining minor observations (non-blocking)

What's working well

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

dognose24 left a comment

Choose a reason for hiding this comment

Uh oh!

dognose24 Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

adamwoodnz Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

kangzj Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

adamwoodnz Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

kangzj Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

adamwoodnz Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

kangzj left a comment

Choose a reason for hiding this comment

Uh oh!

adamwoodnz commented Mar 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

adamwoodnz commented Mar 11, 2026 •

edited

Loading

github-actions bot commented Mar 11, 2026 •

edited

Loading

github-actions bot commented Mar 11, 2026 •

edited

Loading

jp-launch-control bot commented Mar 12, 2026 •

edited

Loading

claude bot commented Mar 12, 2026 •

edited

Loading