-
Notifications
You must be signed in to change notification settings - Fork 835
Charts: Add interactive legend for toggling series visibility #45506
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?
Charts: Add interactive legend for toggling series visibility #45506
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
Adds interactive legend support to toggle series visibility across charts. Key changes introduce visibility state management in the global charts context, wire up interactive legend behaviors with full keyboard/ARIA support, and update chart components to respect hidden series while preserving color consistency via original series indices.
- Add series visibility API to GlobalChartsContext (toggle/isVisible/getHiddenSeries) and implement state via Map<chartId, Set>
- Update LineChart, BarChart, and PieChart to filter hidden series and preserve original indices for consistent colors
- Make Legend items interactive (click and keyboard), with new styles and Storybook story; pass chartId automatically from context
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
projects/js-packages/charts/src/stories/legend-config.tsx | Adds a Storybook argType for enabling interactive legends |
projects/js-packages/charts/src/providers/chart-context/types.ts | Extends GlobalChartsContextValue with methods for series visibility |
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx | Implements visibility state and exposes toggle/isVisible/getHiddenSeries |
projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx | Filters hidden series and preserves original indices for colors |
projects/js-packages/charts/src/components/line-chart/line-chart.tsx | Filters hidden series; uses original indices to keep colors/IDs stable |
projects/js-packages/charts/src/components/legend/types.ts | Adds interactive and chartId props to BaseLegend/Legend props |
projects/js-packages/charts/src/components/legend/stories/index.stories.tsx | Adds InteractiveLegend story demonstrating toggling |
projects/js-packages/charts/src/components/legend/private/base-legend.tsx | Adds interactive behaviors, handlers, ARIA, and styling hooks |
projects/js-packages/charts/src/components/legend/private/base-legend.module.scss | Adds styles for interactive and inactive legend states |
projects/js-packages/charts/src/components/legend/legend.tsx | Passes chartId from context to BaseLegend |
projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx | Filters hidden series; maintains original index for color/pattern consistency |
projects/js-packages/charts/changelog/add-charts-54-legends-toggling-visibility-of-series | Adds changelog entry |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
legendInteractive: { | ||
control: { type: 'boolean' as const }, | ||
table: { category: 'Legend' }, | ||
description: | ||
'Enable interactive legend items that can toggle series visibility. Requires GlobalChartsProvider and chartId to be set.', | ||
}, |
Copilot
AI
Oct 15, 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.
The argType name 'legendInteractive' does not match the actual Legend prop 'interactive'. This prevents the Storybook control from wiring to the component prop. Rename the key to 'interactive' or map it in the story (e.g., pass interactive={legendInteractive}).
Copilot uses AI. Check for mistakes.
const createKeyDownHandler = useCallback( | ||
( labelText: string ) => { | ||
if ( ! interactive ) { | ||
return undefined; | ||
} | ||
return ( event: React.KeyboardEvent ) => { | ||
if ( event.key === 'Enter' || event.key === ' ' ) { | ||
event.preventDefault(); | ||
handleLegendClick( labelText ); | ||
} | ||
}; | ||
}, | ||
[ interactive, handleLegendClick ] |
Copilot
AI
Oct 15, 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.
React.KeyboardEvent is referenced without importing the React namespace, which will cause 'Cannot find namespace React' in TypeScript. Import the type from 'react' and use it directly: add 'type KeyboardEvent' to the react imports and change the annotation to '(event: KeyboardEvent)'.
Copilot uses AI. Check for mistakes.
&--inactive { | ||
opacity: 0.4; | ||
|
||
.legend-item-text { |
Copilot
AI
Oct 15, 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.
The selector targets '.legend-item-text', but the label element in this component uses the existing '.legend-item-label' class. As a result, the strikethrough will not apply. Update the selector to '.legend-item-label'.
.legend-item-text { | |
.legend-item-label { |
Copilot uses AI. Check for mistakes.
|
||
const getHiddenSeries = useCallback( | ||
( chartId: string ) => { | ||
return hiddenSeries.get( chartId ) || new Set(); |
Copilot
AI
Oct 15, 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.
getHiddenSeries returns the internal Set instance from state, allowing external mutation of provider state. Return a defensive copy to avoid accidental mutations: 'const set = hiddenSeries.get(chartId); return set ? new Set(set) : new Set();'.
return hiddenSeries.get( chartId ) || new Set(); | |
const set = hiddenSeries.get( chartId ); | |
return set ? new Set(set) : new Set(); |
Copilot uses AI. Check for mistakes.
{ visibleData.map( seriesData => { | ||
// Find original index to maintain consistent colors | ||
const originalIndex = dataSorted.findIndex( s => s.label === seriesData.label ); | ||
const { color, lineStyles, glyph } = getElementStyles( { | ||
data: seriesData, | ||
index, | ||
index: originalIndex, | ||
} ); |
Copilot
AI
Oct 15, 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.
[nitpick] Computing originalIndex with findIndex inside the map is O(n^2). Precompute a label->index Map once with useMemo and look up in O(1): 'const indexMap = useMemo(() => new Map(dataSorted.map((s, i) => [s.label, i])), [dataSorted]);' then use 'const originalIndex = indexMap.get(seriesData.label) ?? -1;'.
Copilot uses AI. Check for mistakes.
{ dataWithVisibleZeros.map( seriesData => { | ||
// Find original index to maintain consistent colors | ||
const originalIndex = dataSorted.findIndex( s => s.label === seriesData.label ); | ||
return ( | ||
<BarSeries | ||
key={ seriesData?.label } | ||
dataKey={ seriesData?.label } | ||
data={ seriesData.data as DataPointDate[] } | ||
yAccessor={ chartOptions.accessors.yAccessor } | ||
xAccessor={ chartOptions.accessors.xAccessor } | ||
colorAccessor={ getBarBackground( originalIndex ) } | ||
/> | ||
); | ||
} ) } |
Copilot
AI
Oct 15, 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.
[nitpick] Same O(n^2) pattern from findIndex inside a render map. Use a memoized Map of label->originalIndex (built from dataSorted) and replace findIndex with a direct lookup.
Copilot uses AI. Check for mistakes.
const dataWithIndex = visibleData.map( d => { | ||
const originalIndex = data.findIndex( item => item.label === d.label ); | ||
return { | ||
...d, | ||
index: originalIndex, | ||
}; | ||
} ); |
Copilot
AI
Oct 15, 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.
[nitpick] The per-item findIndex is O(n^2). Precompute a Map of original indices: 'const indexMap = useMemo(() => new Map(data.map((item, i) => [item.label, i])), [data]);' then use 'index: indexMap.get(d.label) ?? -1'.
Copilot uses AI. Check for mistakes.
aria-pressed={ interactive ? visible : undefined } | ||
aria-label={ | ||
interactive | ||
? `${ label.text }${ visible ? ', visible' : ', hidden' }. Click to toggle.` |
Copilot
AI
Oct 15, 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.
[nitpick] The aria-label instructs to 'Click to toggle', which excludes keyboard users. Prefer neutral wording like 'Toggle visibility' and state the current status: e.g., ${label.text}: ${visible ? 'visible' : 'hidden'}. Toggle visibility.
? `${ label.text }${ visible ? ', visible' : ', hidden' }. Click to toggle.` | |
? `${label.text}: ${visible ? 'visible' : 'hidden'}. Toggle visibility.` |
Copilot uses AI. Check for mistakes.
Code Coverage SummaryCoverage changed in 5 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
I think it'd be faster if we add this for one chart type. If it works well, we then could easily apply to other charts. What do you think>? |
) ) } | ||
{ dataWithVisibleZeros.map( seriesData => { | ||
// Find original index to maintain consistent colors | ||
const originalIndex = dataSorted.findIndex( s => s.label === seriesData.label ); |
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 think it might be better to remember the original index from the process started L105 rather than finding them dynamically
Proposed changes:
toggleSeriesVisibility
,isSeriesVisible
, andgetHiddenSeries
methods to GlobalChartsContext for managing series visibility stateinteractive
prop to Legend component to enable clickable legend items (defaults to false for backward compatibility)legendInteractive
argType to Storybook configurationOther information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions:
In Storybook:
Backward Compatibility:
interactive
prop continue to work exactly as beforechartId
or GlobalChartsProvider work as standalone legends