Skip to content

Conversation

annacmc
Copy link
Contributor

@annacmc annacmc commented Aug 14, 2025

Fixes CHARTS-90: Add composition API for pie chart
family

Proposed changes:

  • Add composition API support for pie chart family (PieChart, PieSemiCircleChart) using
    <PieChart.Legend /> syntax
  • Fix rendering of SVG vs React children - SVG elements render inside the SVG, React
    components render outside
  • Optimize performance by replacing dual Children.map iterations with a single pass
  • Reorganize Storybook examples to show composition API after traditional prop-based
    examples
  • Maintain full backward compatibility with existing showLegend prop approach

Technical details:

  • Added proper child element handling to separate SVG children (Group components) from
    React children
  • Implemented single-pass child processing using useMemo and Children.forEach for
    better performance
  • Reduced computational complexity from O(2n) to O(n) when processing children
  • Ensured SVG elements like <Group> render inside the SVG context while React components
    render outside
  • Applied consistent patterns across PieChart and PieSemiCircleChart components

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?

Jetpack product discussion

Charts package enhancement - adding composition API for pie chart family to match
LineChart and BarChart functionality, enabling more flexible chart composition patterns.

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

No, this PR only adds a new API pattern for legend composition and optimizes existing
functionality without changing any data handling or tracking.

Testing instructions:

  • Test the new composition API with <PieChart.Legend /> and <PieSemiCircleChart.Legend />
  • Verify backward compatibility with existing showLegend prop approach
  • Test rendering of mixed children (SVG and React components)
  • Verify performance improvements with multiple children
  • Check all Storybook examples render correctly

Specific test cases:

  1. Composition API Usage:
    <PieChart data={data}>
      <PieChart.Legend orientation="horizontal" alignment="center" />
    </PieChart>
  2. Mixed Children (SVG + React):
Center Label 3. Backward Compatibility: 4. PieSemiCircleChart Tests: - Test composition API with various legend configurations - Verify label and note text render correctly - Test with both clockwise and counter-clockwise orientations - Test with custom SVG children 5. Performance Verification: - Test charts with 5-10 child components - Verify no visual regressions - Confirm all existing tests pass - Check that SVG elements render inside the SVG and React components render outside 6. Storybook Verification: - Navigate to Pie Chart and Pie Semi-Circle Chart stories - Verify composition API examples appear after traditional examples - Check that all existing stories continue to work - Verify the renamed "WithCompositionLegend" stories render correctly

@annacmc annacmc requested a review from Copilot August 14, 2025 11:27
Copy link
Contributor

github-actions bot commented Aug 14, 2025

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 add/charts-90-charts-add-composition-api-for-pie-chart-family branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack add/charts-90-charts-add-composition-api-for-pie-chart-family

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

Copy link
Contributor

github-actions bot commented Aug 14, 2025

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 a "[Type]" label (Bug, Enhancement, Janitorial, Task).
  • ✅ 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!

@github-actions 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 Aug 14, 2025
Copilot

This comment was marked as outdated.

Copy link

jp-launch-control bot commented Aug 14, 2025

Code Coverage Summary

Coverage changed in 2 files.

File Coverage Δ% Δ Uncovered
projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx 65/80 (81.25%) -16.93% 14 💔
projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx 75/81 (92.59%) -0.13% 2 ❤️‍🩹

Full summary · PHP report · JS report

Coverage check overridden by Coverage tests to be added later Use to ignore the Code coverage requirement check when tests will be added in a follow-up PR .

@annacmc annacmc changed the title Add/charts 90 charts add composition api for pie chart family Charts: adds legend composition for pie chart family Aug 15, 2025
@annacmc annacmc self-assigned this Aug 15, 2025
@annacmc annacmc requested a review from a team August 15, 2025 11:37
@annacmc annacmc added Coverage tests to be added later Use to ignore the Code coverage requirement check when tests will be added in a follow-up PR and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Aug 15, 2025
@annacmc annacmc marked this pull request as ready for review August 15, 2025 11:37
@annacmc annacmc requested a review from Copilot August 15, 2025 11:40
Copilot

This comment was marked as outdated.

@annacmc annacmc force-pushed the add/charts-90-charts-add-composition-api-for-pie-chart-family branch from 6be5518 to 06c3060 Compare August 18, 2025 11:52
@annacmc annacmc requested a review from Copilot August 18, 2025 11:54
Copy link
Contributor

@Copilot 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 adds a composition API for the pie chart family (PieChart and PieSemiCircleChart) allowing developers to use <PieChart.Legend /> syntax for more flexible chart composition, while maintaining full backward compatibility with existing prop-based approaches.

  • Adds compound component support (PieChart.SVG, PieChart.HTML, PieChart.Legend) for flexible composition
  • Optimizes child processing performance by replacing dual iterations with single-pass processing
  • Reorganizes Storybook examples to demonstrate both traditional and composition API patterns

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pie-chart.tsx Implements composition API with compound components and optimized child processing
pie-semi-circle-chart.tsx Adds composition API support matching PieChart implementation
index.stories.tsx (both) Adds Storybook examples demonstrating composition API usage
donut.stories.tsx Updates donut chart stories with composition API examples
index.docs.mdx Documents the new composition API with usage examples
composition-api.test.tsx Adds comprehensive tests for the new composition functionality
index.tsx Exports both responsive and unresponsive chart variants
changelog file Documents the feature addition

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

</h3>
</PieChartUnresponsive.HTML>

<PieChartUnresponsive.SVG>
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Using PieChartUnresponsive instead of the standard PieChart component creates inconsistency in the API examples. Stories should demonstrate the primary API that users will consume.

Suggested change
<PieChartUnresponsive.SVG>
<PieChart
data={ chartData }
size={ 400 }
withTooltips={ true }
thickness={ 0.7 }
>
<PieChart.HTML>
<h3 style={ { textAlign: 'center', marginBottom: '20px' } }>
Device Usage Distribution
</h3>
</PieChart.HTML>
<PieChart.SVG>

Copilot uses AI. Check for mistakes.

@annacmc annacmc requested a review from kangzj August 19, 2025 01:35
@annacmc annacmc added the [Status] Needs Review This PR is ready for review. label Aug 19, 2025
@github-actions github-actions bot removed the [Status] Needs Review This PR is ready for review. label Aug 19, 2025
@@ -55,6 +58,20 @@ interface PieChartProps extends BaseChartProps< DataPointPercentage[] > {
children?: ReactNode;
}

// Base props type with optional responsive properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's consolidate the code with semi circle chart in follow ups, particularly L61-L129

@@ -120,6 +167,40 @@ const PieChartInternal = ( {

const { isValid, message } = validateData( data );

// Process children to extract compound components
const { svgChildren, htmlChildren, otherChildren } = useMemo( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function could be reused as well in semi circle chart

Children.forEach( child.props.children, htmlChild => {
html.push( htmlChild );
} );
} else if ( child.type === Group ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need this, do we?

Children.forEach( child.props.children, htmlChild => {
html.push( htmlChild );
} );
} else if ( child.type === Group ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

kangzj
kangzj previously approved these changes Aug 19, 2025
Copy link
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 well and looks good! Left a couple of non-blocking comments. Let's ship and iterate 🚀

@annacmc annacmc force-pushed the add/charts-90-charts-add-composition-api-for-pie-chart-family branch from 06c3060 to f2f0171 Compare August 19, 2025 07:37
@annacmc annacmc merged commit cd9e13b into trunk Aug 19, 2025
128 of 137 checks passed
@annacmc annacmc deleted the add/charts-90-charts-add-composition-api-for-pie-chart-family branch August 19, 2025 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Coverage tests to be added later Use to ignore the Code coverage requirement check when tests will be added in a follow-up PR [JS Package] Charts RNA [Tests] Includes Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants