Charts: Add standalone Legend component with Chart Context system #44245
+742
−102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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! |
github-actions
bot
added
the
[Status] Needs Author Reply
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Code Coverage SummaryCoverage changed in 3 files.
2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
annacmc
force-pushed
the
add/charts-50-standalone-legend-component
branch
2 times, most recently
from
ce9977f to
b018ace
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Adds a standalone Legend component and refactors existing chart components to use a common BaseLegend and a new useChartLegendData hook for generating legend items, with optional context integration.
- Introduces
useChartLegendDatahook andBaseLegendcomponent, plus aLegendwrapper for context-aware rendering. - Refactors BarChart, LineChart, PieChart, and PieSemiCircleChart to use the new legend hook and BaseLegend.
- Updates
ChartContextto export and trigger re-renders on chart registration changes.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/src/types.ts | Changed import to import type for AnnotationStyles |
| projects/js-packages/charts/src/providers/chart-context/chart-context.tsx | Exported ChartContext and added state version to trigger updates |
| projects/js-packages/charts/src/index.ts | Exposed Legend, BaseLegend, and useChartLegendData exports |
| projects/js-packages/charts/src/components/shared/with-responsive.tsx | Added overflow: 'hidden' to responsive wrapper style |
| projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx | Swapped Legend for BaseLegend and updated legend comment |
| projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx | Refactored legend logic, added context-aware provider detection |
| projects/js-packages/charts/src/components/pie-chart/pie-chart.module.scss | Added overflow: hidden to .pie-chart class |
| projects/js-packages/charts/src/components/line-chart/line-chart.tsx | Refactored legend logic, hook usage, and context provider wrapping |
| projects/js-packages/charts/src/components/legend/use-chart-legend-data.ts | Added new hook to generate legend items from chart data |
| projects/js-packages/charts/src/components/legend/types.ts | Introduced BaseLegendProps and updated LegendProps |
| projects/js-packages/charts/src/components/legend/stories/index.stories.tsx | Updated stories to demonstrate standalone and context-aware legends |
| projects/js-packages/charts/src/components/legend/legend.tsx | New Legend wrapper component that reads from context or props |
| projects/js-packages/charts/src/components/legend/legend.test.tsx | Added tests for Legend context and fallback behaviors |
| projects/js-packages/charts/src/components/legend/index.ts | Centralized exports of Legend, BaseLegend, hook, and types |
| projects/js-packages/charts/src/components/legend/base-legend.tsx | Updated forwardRef generic to BaseLegendProps |
| projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx | Switched to new legend hook/BaseLegend and context wrapping |
| projects/js-packages/charts/changelog/add-charts-50-standalone-legend-component | Added changelog entry for standalone legend feature |
Comments suppressed due to low confidence (1)
projects/js-packages/charts/src/components/legend/use-chart-legend-data.ts:139
- The new useChartLegendData hook lacks unit tests. Add focused tests to verify item generation for series data, point data, and options combinations.
export function useChartLegendData<
| theme: ChartTheme, | ||
| options: ChartLegendOptions = {} | ||
| ): LegendItemWithGlyph[] | LegendItemWithoutGlyph[] { | ||
| const { showValues = true, withGlyph = false, glyphSize = 8, renderGlyph } = options; |
There was a problem hiding this comment.
Defaulting showValues to true will cause series-based legends (e.g., BarChart, LineChart) to display the count of data points as the value. Consider defaulting showValues to false or requiring explicit enablement to preserve previous behavior.
Suggested change
| const { showValues = true, withGlyph = false, glyphSize = 8, renderGlyph } = options; | |
| const { showValues = false, withGlyph = false, glyphSize = 8, renderGlyph } = options; |
Copilot uses AI. Check for mistakes.
| @@ -30,7 +33,8 @@ export const ChartProvider: FC< ChartProviderProps > = ( { children } ) => { | |||
| unregisterChart, | |||
| getChartData, | |||
| } ), | |||
| [ registerChart, unregisterChart, getChartData ] | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
The lint suppression for react-hooks/exhaustive-deps can be removed by including all actual dependencies (e.g., registerChart, unregisterChart, getChartData, and version) in the dependency array of useMemo.
Suggested change
| // eslint-disable-next-line react-hooks/exhaustive-deps |
Copilot uses AI. Check for mistakes.
- Create ChartLegendOptions interface with showValues, withGlyph, glyphSize, and renderGlyph options - Add ChartLegendProps interface extending LegendProps with chartId for future context integration - Implement useChartLegendData hook with overloads for SeriesData[] and DataPoint[]/DataPointPercentage[] - Support both series data and point data with proper color, shape, and glyph handling - Include valueDisplay preference for DataPointPercentage items - Return properly typed LegendItemWithGlyph[] or LegendItemWithoutGlyph[] arrays
- Create ChartLegend component as a wrapper around BaseLegend - Accept chartId prop for future context integration - Pass through all legend props to BaseLegend for consistent behavior - Enables standalone legend usage independent of chart showLegend prop
- Replace inline legend items creation with useChartLegendData hook - Pass withGlyph and glyphSize options to maintain existing functionality - Eliminates duplicate legend creation logic - Maintains all existing legend behavior and styling
- Replace inline legend items creation with useChartLegendData hook - Add useChartTheme hook to get provider theme for consistent legend colors - Move hook call to proper position to avoid rules-of-hooks violation - Remove duplicate legend creation logic - Maintains all existing legend behavior and styling
- Replace inline legend items creation with useChartLegendData hook in both components - Enable showValues option for both charts to display percentages in legend - Move hook calls to proper position to avoid rules-of-hooks violations - Remove duplicate legend creation logic - Maintain valueDisplay support for properly formatted percentage values - Keep all existing legend behavior and styling
- Export ChartLegend component and useChartLegendData hook from main index - Export ChartLegendProps and ChartLegendOptions types - Add chart-legend module index file with all exports - Makes standalone legend functionality available to consumers
- Create multiple story examples showing different usage patterns - Include stories for standalone usage, chart integration, and alignment options - Add stories demonstrating usage with LineChart, BarChart, and PieChart - Include examples for multiple charts sharing a legend - Add comprehensive test suite for ChartLegend component and useChartLegendData hook - Test both SeriesData and DataPointPercentage data types - Test memoization, glyph handling, and value display functionality - Fix TypeScript any usage in mock functions
- Resolve merge conflicts from chart context foundation PR - Fix duplicate legendItems declarations in PieChart and PieSemiCircleChart - Maintain custom color handling in PieSemiCircleChart using accessors - Fix type-only import for AnnotationStyles - Fix duplicate identifier in ChartLegend stories
- Add chart context integration to automatically retrieve legend data - Support chartId prop for standalone usage - Maintain fallback to items prop for manual data passing - Add story demonstrating standalone usage with chartId - Component works with showLegend=false charts - Accepts all BaseLegend props through inheritance
- Create validateSeriesData for line/bar charts - Create validatePercentageData for pie charts - Create validateSemiCircleData for semi-circle charts - Create validateBarChartData for bar-specific validation - Eliminates ~60 lines of duplicated validation code
- Enhances existing Legend to support chartId prop - Automatically retrieves legend data from chart context - Falls back to manual items prop for backward compatibility - Enables standalone legend usage with <Legend chartId="my-chart" />
- Extract formatPointValue for consistent value formatting - Extract createBaseLegendItem for common legend item creation - Extract processSeriesData and processPointData functions - Eliminates ~50 lines of duplicated mapping logic - Improves code readability and testability
- Split LegendProps into BaseLegendProps and LegendProps - BaseLegendProps for direct usage (items required) - LegendProps for context usage (items optional, chartId added) - Export new Legend component alongside BaseLegend - Export useChartLegendData and ChartLegendOptions
- Export ChartContext to enable direct useContext usage - Allows Legend component to handle missing provider gracefully - Enables flexible context usage patterns
- LineChart: Use validateSeriesData instead of custom validation - BarChart: Use validateBarChartData for label requirements - PieChart: Use validatePercentageData with 100% requirement - PieSemiCircleChart: Use validateSemiCircleData for >0% requirement - Update imports to use BaseLegend for internal chart usage - Eliminates ~60 lines of duplicated validation code
- Eliminates technical debt from duplicate component - Consolidates legend functionality into enhanced Legend component - Maintains clean architecture with single legend implementation
- Export enhanced Legend and BaseLegend components - Export useChartLegendData from legend module - Export LegendProps, BaseLegendProps, ChartLegendOptions types - Remove ChartLegend references in favor of unified Legend
- Replace basic legend stories with enhanced documentation - Add context integration examples with chartId usage - Add real-world dashboard layout example - Document standalone legend capabilities - Include code examples and usage patterns - Demonstrate both manual and automatic legend data
- Add missing newline at end of file - Fix JSDoc formatting consistency
This reverts commit 5f69ab2.
This reverts commit 1995652.
- Update chart components to import from correct legend paths - Change Legend to BaseLegend for internal chart usage - Fix import order to comply with ESLint rules 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Change ChartProvider from useRef to useState for chart storage - Ensures Legend component re-renders when chart data is registered - Fixes standalone legend not displaying in Storybook
- Use version counter approach with useRef for chart storage - Increment version when charts are registered/unregistered - Prevents infinite loops while maintaining reactivity
Adds tests for the Legend wrapper component covering: - Direct items prop usage - Context-based data retrieval via chartId - Fallback behavior when context data is unavailable - Null rendering for edge cases - Prop forwarding to BaseLegend - Context data prioritization All 8 new tests pass, bringing total legend test coverage to 17 tests.
The Legend component was not re-rendering when chart data changed in the context because it wasn't properly subscribing to context changes. Added useMemo with context as a dependency to ensure the component re-renders when the chart context updates. This fixes the issue where standalone Legend components would not display data from charts registered in the same ChartProvider context.
The LineChart component was always creating its own ChartProvider, which prevented standalone Legend components from accessing chart data when both were wrapped in the same ChartProvider. Modified LineChart to only create a new ChartProvider when it's not already inside an existing one, allowing it to share context with other components.
Similar to LineChart, the PieChart component was always creating its own ChartProvider, which prevented standalone Legend components from accessing chart data when both were wrapped in the same ChartProvider. Modified PieChart to only create a new ChartProvider when it's not already inside an existing one, allowing it to share context with other components.
Improved legend readability by adding a space between the label text and value (e.g., "Desktop 65%" instead of "Desktop65%"). This makes the legend items more visually clear and easier to read.
Similar to LineChart and PieChart, the BarChart component was always creating its own ChartProvider, which prevented standalone Legend components from accessing chart data when both were wrapped in the same ChartProvider. Modified BarChart to only create a new ChartProvider when it's not already inside an existing one, allowing it to share context with other components. This fixes the "Sales by Quarter" chart legend items not showing in the dashboard story.
Added overflow: hidden to both the PieChart CSS and the withResponsive HOC container to prevent chart content from flowing out of its designated container bounds. This fixes the issue where pie charts in constrained layouts (like the dashboard story) would visually overflow their containers.
Changed the default value of showValues from true to false in useChartLegendData hook to preserve the previous behavior where SeriesData legends don't show data counts by default. This prevents unintended display of data point counts in legends for LineChart and BarChart components. Addresses Copilot feedback about preserving existing functionality.
The version dependency is needed to trigger re-renders when charts are registered/unregistered, but ESLint can't detect this relationship. Added a comment explaining why the lint suppression is necessary. The useMemo needs to recreate the context value when the version changes (via registerChart/unregisterChart calls) to ensure components using the context re-render with updated chart data.
Added focused unit tests for the useChartLegendData hook to verify legend item generation for different data types and configurations. Tests cover: - SeriesData handling (with and without values) - DataPointPercentage handling (with and without values) - Glyph handling (with and without glyphs) - Edge cases (empty, null, undefined data) - Memoization behavior - Theme color cycling - Default option values Addresses Copilot feedback about adding focused tests for the hook.
1. Fix Legend ref prop error in bar-chart.tsx: - Wrapped Legend component in div with ref since Legend doesn't support ref prop - This maintains legend height measurement functionality 2. Remove textColor from ChartTheme mock in test file: - textColor property doesn't exist in ChartTheme type - Removed from mock data to fix type error
gridColorDark properties to the ChartTheme mock in the test file
annacmc
force-pushed
the
add/charts-50-standalone-legend-component
branch
from
b76937a to
2b9e9e9
Compare
annacmc
added
the
Coverage tests to be added later
- Remove unused TooltipContext and useEffect imports from line-chart.tsx - Remove unused useState import and version state from chart-context.tsx - Completes ESLint cleanup after removing dead code
annacmc
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #50
Proposed changes:
Key Features:
<Legend items={[...]} />for custom legend items<Legend chartId="my-chart" />to display legend for a specific chartOther information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No data tracking changes.
Testing instructions:
1. Test Standalone Legend Component