-
Notifications
You must be signed in to change notification settings - Fork 822
Update/charts 49 chart context foundation #44189
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
base: trunk
Are you sure you want to change the base?
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! |
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
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 lays the groundwork for a new ChartContext system, enabling charts to register themselves and share metadata across components.
- Introduce an optional
chartId
prop and auto-generated fallback - Add
ChartProvider
,useChartContext
,useChartId
, anduseChartRegistration
utilities - Integrate context registration into existing chart components (Pie, Semi-Pie, Line, Bar)
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 | Added optional chartId to base chart props |
projects/js-packages/charts/src/providers/chart-context/utils.ts | Implemented useChartId and useChartRegistration hooks |
projects/js-packages/charts/src/providers/chart-context/types.ts | Defined types for chart registration entries |
projects/js-packages/charts/src/providers/chart-context/chart-context.tsx | Created ChartProvider and useChartContext |
projects/js-packages/charts/src/providers/chart-context/chart-context.test.tsx | Added tests for context hooks |
projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx | Wrapped component in ChartProvider and register via context |
projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx | Wrapped component in ChartProvider and register via context |
projects/js-packages/charts/src/components/line-chart/line-chart.tsx | Wrapped component in ChartProvider and register via context; updated ID handling |
projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx | Wrapped component in ChartProvider and register via context; updated ID handling |
projects/js-packages/charts/changelog/update-charts-49-chart-context-foundation | Added changelog entry for chart context foundation |
Comments suppressed due to low confidence (1)
projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx:123
- Add a test case for the
PieChart
component to verify that it correctly registers itself with the context when rendered, and unregisters on unmount.
useChartRegistration( chartId, legendItems, providerTheme, 'pie', {
projects/js-packages/charts/src/providers/chart-context/chart-context.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx
Outdated
Show resolved
Hide resolved
76beea3
to
29635bb
Compare
Define ChartRegistration interface for chart data storage and ChartContextValue interface for context API operations. Sets up foundation for chart registry system.
Create ChartProvider component with Map-based registry for chart data. Add useChartContext hook with error boundary for outside usage. Implement registerChart, unregisterChart, and getChartData methods.
Add useChartId hook for ID generation and resolution. Add useChartRegistration hook for chart lifecycle management. Support auto-generated IDs via React.useId() for unique identification.
Export ChartProvider and useChartContext from chart-context module. Export utility hooks useChartId and useChartRegistration. Export TypeScript types for external usage.
Add chartId prop for user-provided chart identifiers. Update JSDoc documentation for the new property. Maintains full backward compatibility with existing usage.
Wrap BarChart with ChartProvider for context registration. Register chart data with legend items, theme, and metadata on mount. Use memoized legend items for performance optimization. Maintain separate internal ID for pattern generation.
Wrap LineChart with ChartProvider for context registration. Register chart data including glyph options and gradient settings. Use memoized legend items with proper dependencies. Maintain separate internal ID for gradient generation.
Wrap PieChart with ChartProvider for context registration. Register chart data with metadata including thickness and gap options. Use memoized legend items for performance optimization. Support chart-specific configuration options in metadata.
Wrap PieSemiCircleChart with ChartProvider for context registration. Register chart data with semi-circle specific metadata options. Use memoized legend items with accessor dependencies. Support clockwise and thickness configuration in metadata.
Test provider functionality and error boundaries. Test chart registration and unregistration lifecycle. Test multiple chart independence and ID generation. Verify function reference stability and collision handling.
- Use provider theme instead of XY theme for registration - Fix TypeScript types in chart context tests - Fix ESLint import order and formatting issues 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add metadata memoization in useChartRegistration to prevent unnecessary re-renders - Add displayName to LineChart and BarChart for better debugging in React DevTools - Maintain all necessary React hook imports (useContext, useEffect are used) 🤖 Generated with [Claude Code](https://claude.ai/code)
- Add isDataValid parameter to useChartRegistration hook - Only register charts with valid data to prevent invalid context entries - Maintain React hooks rules by calling all hooks in same order every render - Apply to all chart components: BarChart, LineChart, PieChart, PieSemiCircleChart 🤖 Generated with [Claude Code](https://claude.ai/code)
…context.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
16aefa0
to
d79adff
Compare
Fixes #
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: