Charts: Make PieSemiCircleChart responsive by default by adamwoodnz · Pull Request #47312 · Automattic/jetpack

adamwoodnz · 2026-02-25T01:21:20Z

Fixes: https://linear.app/a8c/issue/CHARTS-169

Proposed changes:

Make PieSemiCircleChart responsive by default — fills its parent container when no explicit width/height is provided
Maintain strict 2:1 width-to-height ratio, constrained by available space using useElementSize on the SVG wrapper
Apply the same constrained sizing to the error-state SVG so it respects propHeight and doesn't overflow short containers
Replace manual legend-top flex hack with @wordpress/ui Stack for layout; add configurable gap prop
Move gap prop to shared BaseChartProps and remove duplicate GapSize imports from BarChart, LineChart, and PieChart
Simplify Storybook stories to use responsive defaults: remove redundant Responsiveness story, drop hardcoded width/containerWidth props, add height control and fixed-dimensions story
Update tests for responsive behaviour including height-constrained scenarios

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Testing instructions:

Start Storybook: cd projects/js-packages/charts && pnpm storybook
Open the PieSemiCircleChart stories
Verify the default story fills its container responsively and maintains the 2:1 ratio when resized
Verify the fixed-dimension story renders at the specified size and does not resize
Confirm legend renders correctly for both top and bottom positions
Check error states render correctly and don't overflow their containers
Run unit tests: cd projects/js-packages/charts && pnpm test

Changelog

See projects/js-packages/charts/changelog/charts-169-fix-chart-height-and-size-calculation-for-pie-semi-circle-chart

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

No. This PR only changes how PieSemiCircleChart calculates and renders its dimensions — no data collection, tracking, or analytics are added or modified.

…ments. Update BarChart, LineChart, and PieChart interfaces to include the new prop, and reflect this change in their respective documentation. Remove redundant gap prop definitions from the interfaces.

The chart previously hardcoded a 400px width and derived height by subtracting legend height from the chart area. This caused incorrect sizing when the container had height constraints or the legend position changed. The chart now fills its parent container and measures the available SVG wrapper area to maintain a 2:1 width-to-height ratio, constrained by both available width and height. Explicit width/height props override the responsive behavior. - Replace fixed width default with responsive container measurement - Use @wordpress/ui Stack for gap spacing between chart elements - Add configurable gap prop using design system tokens - Wrap SVG in a flex measurement div for accurate sizing Co-authored-by: Cursor <cursoragent@cursor.com>

github-actions · 2026-02-25T01:22:13Z

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-169-fix-chart-height-and-size-calculation-for-pie-semi-circle branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack charts-169-fix-chart-height-and-size-calculation-for-pie-semi-circle

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-02-25T01:22:23Z

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!

Copilot

Pull request overview

This PR makes PieSemiCircleChart responsive by default, so it fills its parent container when no explicit dimensions are provided, while preserving a strict 2:1 width-to-height ratio based on available space. It also standardizes inter-element spacing via a shared gap prop and updates Storybook/docs/tests accordingly.

Changes:

Implement responsive-by-default sizing for PieSemiCircleChart using measured available space and a constrained 2:1 ratio.
Replace the legend layout hack with @wordpress/ui Stack, and introduce a shared gap prop on BaseChartProps.
Update Storybook stories/docs and adjust unit tests for the new responsive behavior.

Reviewed changes

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

Show a summary per file

File	Description
projects/js-packages/charts/src/types.ts	Adds shared `gap` prop to `BaseChartProps` so charts can use a consistent spacing API.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/test/pie-semi-circle-chart.test.tsx	Updates tests to reflect responsive-by-default behavior and constrained sizing.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/stories/index.stories.tsx	Updates default story to be responsive and adds a fixed-dimensions story using the unresponsive export.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/stories/index.api.mdx	Updates API docs for responsive width behavior and documents the new `gap` prop.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx	Implements measured sizing, 2:1 constraint logic, and Stack-based layout with configurable `gap`.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.module.scss	Adds responsive/container-filling styles and an SVG wrapper that can be measured and flexed.
projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx	Removes duplicated `gap` typing now that it’s in `BaseChartProps`.
projects/js-packages/charts/src/charts/line-chart/types.ts	Removes duplicated `gap` typing now that it’s in `BaseChartProps`.
projects/js-packages/charts/src/charts/line-chart/stories/index.api.mdx	Documents `gap` for LineChart API.
projects/js-packages/charts/src/charts/bar-chart/stories/index.api.mdx	Documents `gap` for BarChart API.
projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx	Removes duplicated `gap` typing now that it’s in `BaseChartProps`.

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

projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx

projects/js-packages/charts/src/charts/pie-semi-circle-chart/stories/index.api.mdx

…onsive fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Correct docs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

jp-launch-control · 2026-02-25T01:49:40Z

Code Coverage Summary

Coverage changed in 1 file.

File	Coverage	Δ%	Δ Uncovered
projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx	69/70 (98.57%)	0.11%	0 💚

Full summary · PHP report · JS report

The invalid-data render path hardcoded width and ignored propHeight, so the error SVG could overflow a height-constrained container. Apply the same 2:1 aspect-ratio constraint used by the valid chart. Co-authored-by: Cursor <cursoragent@cursor.com>

Stories were still using fixed widths and container overrides from the old sizing model. Now that the chart is responsive by default, most stories no longer need explicit width/containerWidth props. - Add height range control to Storybook argTypes - Remove redundant Responsiveness story (all stories are responsive) - Simplify composition and interactive legend examples - Use height prop instead of width where a size constraint is needed Co-authored-by: Cursor <cursoragent@cursor.com>

The flex/column properties are now handled by the Stack component, and margin declarations have no effect on SVG <Text> elements. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

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

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

Copilot · 2026-02-25T02:32:50Z

...ects/js-packages/charts/src/charts/pie-semi-circle-chart/test/pie-semi-circle-chart.test.tsx

+// Mock useParentSize so the responsive wrapper returns predictable dimensions in tests
+jest.mock( '@visx/responsive', () => ( {
+	useParentSize: jest.fn( () => ( {
+		parentRef: { current: null },
+		width: 400,
+		height: 200,
+	} ) ),


The mock is mocking useParentSize from @visx/responsive, which is used by the withResponsive HOC that wraps the component. However, the component also uses useElementSize internally (line 190 in the implementation) for additional dimension constraints. In test environments, useElementSize typically returns 0,0 unless ResizeObserver is mocked, so the component falls back to using propWidth/propHeight passed from the HOC. While this may work for the current tests, it doesn't actually test the full responsive behavior including the useElementSize logic. Consider also mocking ResizeObserver or useElementSize to test the complete responsive sizing behavior, including height-constrained scenarios.

Suggested change

// Mock useParentSize so the responsive wrapper returns predictable dimensions in tests

jest.mock( '@visx/responsive', () => ( {

useParentSize: jest.fn( () => ( {

parentRef: { current: null },

width: 400,

height: 200,

} ) ),

// Mock responsive hooks so the wrapper and internal sizing logic return predictable dimensions in tests

jest.mock( '@visx/responsive', () => ( {

useParentSize: jest.fn( () => ( {

parentRef: { current: null },

width: 400,

height: 200,

} ) ),

useElementSize: jest.fn( () => ( {

ref: { current: null },

width: 300,

height: 100,

} ) ),

Copilot · 2026-02-25T02:32:50Z

...ects/js-packages/charts/src/charts/pie-semi-circle-chart/test/pie-semi-circle-chart.test.tsx

+// Mock useParentSize so the responsive wrapper returns predictable dimensions in tests
+jest.mock( '@visx/responsive', () => ( {
+	useParentSize: jest.fn( () => ( {
+		parentRef: { current: null },
+		width: 400,
+		height: 200,
+	} ) ),
+} ) );
+


The test is mocking useParentSize from @visx/responsive, but the new implementation uses useElementSize from the local hooks (which uses ResizeObserver). This mock is now incorrect and won't actually affect the component behavior. The mock should either be removed, or if predictable dimensions are needed for tests, useElementSize should be mocked instead, or ResizeObserver should be mocked.

Suggested change

// Mock useParentSize so the responsive wrapper returns predictable dimensions in tests

jest.mock( '@visx/responsive', () => ( {

useParentSize: jest.fn( () => ( {

parentRef: { current: null },

width: 400,

height: 200,

} ) ),

} ) );

Copilot · 2026-02-25T02:32:50Z

...ects/js-packages/charts/src/charts/pie-semi-circle-chart/test/pie-semi-circle-chart.test.tsx

+			const wrapper = screen.getByTestId( 'responsive-wrapper' );
+			expect( wrapper ).toHaveStyle( { height: '100%' } );


This test is checking for the 'responsive-wrapper' test ID, which comes from the withResponsive HOC wrapper. However, the PR's main change is to make the component internally responsive using useElementSize and the Stack component with responsive styling. To properly test the new responsive behavior, this test should check the Stack element (test ID 'pie-chart-container') for the 'pie-semi-circle-chart--responsive' class and the height: 100% style when no width/height props are provided. The current test is validating the HOC's behavior, not the component's new internal responsive implementation.

Suggested change

const wrapper = screen.getByTestId( 'responsive-wrapper' );

expect( wrapper ).toHaveStyle( { height: '100%' } );

const container = screen.getByTestId( 'pie-chart-container' );

expect( container ).toHaveClass( 'pie-semi-circle-chart--responsive' );

expect( container ).toHaveStyle( { height: '100%' } );

Copilot · 2026-02-25T02:32:50Z

projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx

 	const prefersReducedMotion = usePrefersReducedMotion();

 	if ( ! isValid ) {
+		const errorWidth = Math.min( propWidth || 400, ( propHeight || ( propWidth || 400 ) / 2 ) * 2 );


The error width calculation has a subtle logic issue. When only propHeight is provided, the expression ( propHeight || ( propWidth || 400 ) / 2 ) evaluates to propHeight, then propHeight * 2 could exceed the default width of 400. For example, if propHeight=400, errorWidth would be min(400, 800) = 400, and errorHeight would be 200, which is smaller than the specified propHeight of 400. This might not match user expectations when they've explicitly set a height constraint. Consider: const errorWidth = Math.min( propWidth || ( propHeight ? propHeight * 2 : 400 ), propHeight ? propHeight * 2 : ( propWidth || 400 ) ); to be more explicit about the intent.

Suggested change

const errorWidth = Math.min( propWidth || 400, ( propHeight || ( propWidth || 400 ) / 2 ) * 2 );

const errorWidth = Math.min(

propWidth || ( propHeight ? propHeight * 2 : 400 ),

propHeight ? propHeight * 2 : ( propWidth || 400 )

);

adamwoodnz and others added 2 commits February 25, 2026 14:15

Enhance chart components by adding 'gap' prop for spacing between ele…

21cef3a

…ments. Update BarChart, LineChart, and PieChart interfaces to include the new prop, and reflect this change in their respective documentation. Remove redundant gap prop definitions from the interfaces.

Copilot AI review requested due to automatic review settings February 25, 2026 01:21

adamwoodnz added the [Status] In Progress label Feb 25, 2026

Copilot started reviewing on behalf of adamwoodnz February 25, 2026 01:21 View session

github-actions bot added [JS Package] Charts [Tests] Includes Tests RNA labels Feb 25, 2026

Copilot AI reviewed Feb 25, 2026

View reviewed changes

projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx Outdated Show resolved Hide resolved

projects/js-packages/charts/src/charts/pie-semi-circle-chart/stories/index.api.mdx Outdated Show resolved Hide resolved

adamwoodnz self-assigned this Feb 25, 2026

adamwoodnz marked this pull request as draft February 25, 2026 01:29

changelog: add changelog entry for CHARTS-169 PieSemiCircleChart resp…

3ce083e

…onsive fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adamwoodnz force-pushed the charts-169-fix-chart-height-and-size-calculation-for-pie-semi-circle branch from 858a52d to 3ce083e Compare February 25, 2026 01:32

Apply suggestion from @Copilot

155464c

Correct docs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

adamwoodnz and others added 3 commits February 25, 2026 15:03

style(charts): Remove redundant SCSS from semi-circle chart

f2fd220

The flex/column properties are now handled by the Stack component, and margin declarations have no effect on SVG <Text> elements. Co-authored-by: Cursor <cursoragent@cursor.com>

adamwoodnz changed the title ~~fix(charts): Make PieSemiCircleChart responsive by default~~ Charts: Make PieSemiCircleChart responsive by default Feb 25, 2026

adamwoodnz marked this pull request as ready for review February 25, 2026 02:23

adamwoodnz requested review from a team and Copilot February 25, 2026 02:23

Copilot started reviewing on behalf of adamwoodnz February 25, 2026 02:24 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Charts: Make PieSemiCircleChart responsive by default#47312

Charts: Make PieSemiCircleChart responsive by default#47312
adamwoodnz wants to merge 7 commits intotrunkfrom
charts-169-fix-chart-height-and-size-calculation-for-pie-semi-circle

adamwoodnz commented Feb 25, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Feb 25, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Feb 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

jp-launch-control bot commented Feb 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		const wrapper = screen.getByTestId( 'responsive-wrapper' );
		expect( wrapper ).toHaveStyle( { height: '100%' } );

-			const wrapper = screen.getByTestId( 'responsive-wrapper' );
-			expect( wrapper ).toHaveStyle( { height: '100%' } );
+			const container = screen.getByTestId( 'pie-chart-container' );
+			expect( container ).toHaveClass( 'pie-semi-circle-chart--responsive' );
+			expect( container ).toHaveStyle( { height: '100%' } );

Comments

Conversation

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

Uh oh!

Proposed changes:

Other information:

Testing instructions:

Changelog

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

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

adamwoodnz commented Feb 25, 2026 •

edited

Loading

github-actions bot commented Feb 25, 2026 •

edited

Loading

github-actions bot commented Feb 25, 2026 •

edited

Loading

jp-launch-control bot commented Feb 25, 2026 •

edited

Loading