-
Couldn't load subscription status.
- Fork 837
Charts: Simplify PieSemiCircleChart API to use children composition #45369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Charts: Simplify PieSemiCircleChart API to use children composition #45369
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this 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 removes the label/note props from PieSemiCircleChart and standardizes content rendering via a children composition API, aligning it with PieChart. It updates tests and stories accordingly and removes unused styles.
- Remove label and note props and switch to children-based composition
- Update tests and stories to render text via visx Group/Text and compound components (SVG/HTML/Legend)
- Remove unused SCSS for label/note
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/src/providers/chart-context/stories/index.stories.tsx | Storybook provider stories updated to render chart headings via children using Group/Text. |
| projects/js-packages/charts/src/components/pie-semi-circle-chart/test/pie-semi-circle-chart.test.tsx | Tests migrated from label/note props to children composition; added compound component coverage. |
| projects/js-packages/charts/src/components/pie-semi-circle-chart/stories/index.stories.tsx | Component stories updated to demonstrate children composition and compound components. |
| projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx | Removed label/note props and their rendering; documented composition API usage on children prop. |
| projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.module.scss | Removed unused .label and .note styles. |
| projects/js-packages/charts/changelog/charts-65-iterate-over-pie-and-piesemicircle-composition | Changelog entry added for composition improvements. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Significance: minor | ||
| Type: changed | ||
|
|
||
| Charts: improve pie semi circle chart composition |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the label and note props is a breaking API change. Please mark this as a breaking change in the changelog (e.g., Significance: major and/or Type: breaking-change) and include a brief migration note showing how to render labels via children (Group/Text or PieSemiCircleChart.SVG).
| Significance: minor | |
| Type: changed | |
| Charts: improve pie semi circle chart composition | |
| Significance: major | |
| Type: breaking-change | |
| Charts: improve pie semi circle chart composition | |
| Breaking change: The `label` and `note` props have been removed from PieSemiCircleChart. To render labels, use children components such as `<Group><Text /></Group>` or render SVG elements directly via `PieSemiCircleChart.SVG`. | |
| Migration note: | |
| Before: | |
| <PieSemiCircleChart label="My Label" note="Some note" ... /> | |
| After: | |
| <PieSemiCircleChart ...> | |
| <Group> | |
| <Text>My Label</Text> | |
| <Text>Some note</Text> | |
| </Group> | |
| </PieSemiCircleChart> | |
| // Or use PieSemiCircleChart.SVG for custom SVG rendering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the semi-circle chart label and note props are currently being used by any consumers, so I don't think it needs to be a significant/major change in the changelog. I'm open to having my mind changed on this, but at this time I don't think this feedback is necessary.
projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx
Show resolved
Hide resolved
.../js-packages/charts/src/components/pie-semi-circle-chart/test/pie-semi-circle-chart.test.tsx
Outdated
Show resolved
Hide resolved
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
fc34507 to
21d2e47
Compare
There was a problem hiding this 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 8 out of 8 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.
|
I've spent a while reviewing these changes, and confirmed they would be breaking for woo analytics. I don't think at this time the small benefits are worth pushing forward with this PR and I think that it would be better as a part of a larger tidy up / refactor later. So closing this up in the meantime. |
Proposed changes:
labelandnoteprops from PieSemiCircleChart componentchildrencomposition pattern like PieChartOther information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions: