Skip to content

Charts: Make PieSemiCircleChart responsive by default#47312

Merged
adamwoodnz merged 10 commits intotrunkfrom
charts-169-fix-chart-height-and-size-calculation-for-pie-semi-circle
Feb 26, 2026
Merged

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

Conversation

@adamwoodnz
Copy link
Copy Markdown
Contributor

@adamwoodnz adamwoodnz commented Feb 25, 2026

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

Summary

This PR is one in a series that make chart components responsive by default, so they fill their parent container without requiring explicit dimensions.

The PR applies the same pattern used for Line, Bar and Pie charts to the PieSemiCircleChart. It also standardises the gap prop across all chart types and aligns the Storybook docs with the responsive-by-default conventions established in the earlier PRs.

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 explicit width and/or height props (e.g. height={400} correctly derives width=800)
  • 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
  • Rewrite Storybook docs to describe responsive-by-default behaviour consistently with line, bar, and pie chart docs — width/height are constraints, not enablers
  • Fix <Source> code blocks in docs to match their <Canvas> stories (data, props, responsive defaults)
  • Fix broken Responsiveness story reference in docs; add FixedDimensions Canvas
  • Add missing height prop to the API reference
  • Simplify Storybook stories: 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:

  1. Start Storybook: cd projects/js-packages/charts && pnpm storybook
  2. Open the PieSemiCircleChart stories
  3. Verify the default story fills its container responsively and maintains the 2:1 ratio when resized
  4. Verify the fixed-dimension story renders at the specified size and does not resize
  5. Confirm legend renders correctly for both top and bottom positions
  6. Check error states render correctly and don't overflow their containers
  7. 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.

adamwoodnz and others added 2 commits February 25, 2026 14:15
…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>
Copilot AI review requested due to automatic review settings February 25, 2026 01:21
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 25, 2026

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
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 25, 2026

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. 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!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@adamwoodnz adamwoodnz self-assigned this Feb 25, 2026
@adamwoodnz adamwoodnz marked this pull request as draft February 25, 2026 01:29
…onsive fix

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@adamwoodnz 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
Correct docs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jp-launch-control
Copy link
Copy Markdown

jp-launch-control bot commented Feb 25, 2026

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 71/72 (98.61%) 0.15% 0 💚

Full summary · PHP report · JS report

adamwoodnz and others added 3 commits February 25, 2026 15:03
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>
@adamwoodnz adamwoodnz changed the title fix(charts): Make PieSemiCircleChart responsive by default Charts: Make PieSemiCircleChart responsive by default Feb 25, 2026
@adamwoodnz adamwoodnz marked this pull request as ready for review February 25, 2026 02:23
@adamwoodnz adamwoodnz requested review from a team and Copilot February 25, 2026 02:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

adamwoodnz and others added 2 commits February 25, 2026 21:53
The error-state SVG ignored an explicit height prop, always defaulting
to 400px width regardless. When only propHeight was provided (e.g. 400),
the error rendered at 400×200 instead of the expected 800×400.

Extracts the repeated `propWidth || 400` fallback into a DEFAULT_WIDTH
constant and effectiveWidth variable, then reworks the error-state
calculation so an explicit height correctly derives width via the 2:1
aspect ratio.

Co-authored-by: Cursor <cursoragent@cursor.com>
The docs described responsive as opt-in ("omit width to be
responsive") when the chart is now responsive by default. This
was inconsistent with line, bar, and pie chart docs which all
frame it as "fills parent container, use width/height to
constrain."

Rewrites the responsive section to match the shared pattern,
fixes Source blocks to match their Canvas stories, adds the
missing height prop to the API reference, and fixes a broken
Responsiveness story reference.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI review requested due to automatic review settings February 25, 2026 09:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.


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

} }
>
<div
<Stack
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Contributor

@kangzj kangzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected and all tests pass 👍

Great efforts aligning the approach 👍 👍 👍

@adamwoodnz adamwoodnz merged commit 4f5f539 into trunk Feb 26, 2026
92 checks passed
@adamwoodnz adamwoodnz deleted the charts-169-fix-chart-height-and-size-calculation-for-pie-semi-circle branch February 26, 2026 02:47
@github-actions github-actions bot added [Status] UI Changes Add this to PRs that change the UI so documentation can be updated. and removed [Status] In Progress labels Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[JS Package] Charts RNA [Status] UI Changes Add this to PRs that change the UI so documentation can be updated. [Tests] Includes Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants