-
Notifications
You must be signed in to change notification settings - Fork 826
Line chart - custom annotations #44131
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
Conversation
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 SummaryCoverage changed in 3 files.
3 files are newly checked for coverage.
|
0cb5041
to
c4bf347
Compare
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
5772d4a
to
f4ab227
Compare
<div className={ styles[ 'line-chart__annotation-label' ] }> | ||
<button | ||
ref={ buttonRef } | ||
{ ...( { popovertarget: popoverId } as ButtonWithPopover ) } |
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.
Using the popover API for these rather than a modal as it has:
Built-in Accessibility: The popover API automatically handles:
- Focus management
- ARIA attributes
- Keyboard navigation (Escape key)
- Screen reader announcements
Browser Behavior: Native handling of:
- Click outside to close
- Proper z-index stacking
- Scroll behavior
- Mobile touch interactions
Browser support is good: https://caniuse.com/mdn-api_htmlelement_popover
height: `${ POPOVER_BUTTON_SIZE }px`, | ||
transform: `translate(${ POPOVER_BUTTON_SIZE / 2 }px, 0)`, | ||
} } | ||
aria-label={ title || 'View details' } |
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 found some other instances where we have static untranslated strings, should we be using @wordpress/i18n
?
eg.
import { __ } from '@wordpress/i18n';
...
aria-label={ title ?? __( 'See details', 'automattic-charts' ) }
...
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 agree - we need these translated for sure.
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.
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 adds support for fully customizable annotations on the LineChart component, including optional HTML labels and popovers.
- Introduce
renderLabel
andrenderLabelPopover
props and refactor annotation rendering to useHtmlLabel
and an external overlay. - Add a
LineChartAnnotationsOverlay
component and expose chart scales/dimensions viaforwardRef
for accurate positioning. - Update tests and Storybook stories to cover the new custom annotation and popover capabilities.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
projects/js-packages/charts/src/types.ts | Add Popover API attribute type definitions |
projects/js-packages/charts/src/components/shared/utils.ts | Add isSafari helper |
projects/js-packages/charts/src/components/line-chart/line-chart.tsx | Refactor to support forwarded ref and external overlay |
projects/js-packages/charts/src/components/line-chart/line-chart-annotation.tsx | Add renderLabel /renderLabelPopover and HTML labels |
projects/js-packages/charts/src/components/line-chart/line-chart-annotations-overlay.tsx | New overlay component |
projects/js-packages/charts/src/components/line-chart/line-chart-annotation-label-popover.tsx | New popover button & positioning logic |
projects/js-packages/charts/src/components/line-chart/test/* | Update and add tests for custom annotations |
projects/js-packages/charts/src/components/line-chart/stories/annotation.stories.tsx | Add stories for custom annotations |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
projects/js-packages/charts/src/components/line-chart/line-chart-annotation.tsx:47
- [nitpick] The new
renderLabel
andrenderLabelPopover
props lack JSDoc comments. Consider adding descriptions for these hooks inLineChartAnnotationProps
to clarify their purpose and usage.
renderLabel?: FC< { title: string; subtitle?: string } >;
projects/js-packages/charts/src/components/line-chart/line-chart-annotation.tsx:162
- [nitpick] The default vertical anchor was changed from
'middle'
to'start'
, which may misalign existing annotations. Verify whether this change is intentional or should be reverted to preserve the previous positioning.
return 'start';
Hey @adamwoodnz ! I've been working on a chart context feature that Currently, chart context allows legends to be rendered independently from I'm wondering if we should explore extending this pattern to annotations as // Current approach
// Potential context approach
This could enable things like shared annotations across multiple charts, or What are your thoughts? Worth exploring, or do you think the current prop-based |
Yeah I certainly think it's worth exploring, I'll take a look at your PR 👍 |
{ /* Render annotations as external overlay to avoid interaction blocking */ } | ||
{ annotations?.length && ( | ||
<LineChartAnnotationsOverlay | ||
chartRef={ internalChartRef } | ||
annotations={ annotations } | ||
chartWidth={ width } | ||
chartHeight={ height - ( showLegend ? legendHeight : 0 ) } | ||
/> | ||
) } |
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 XYChart renders a rect
interaction layer over the whole chart which is used for tooltips, etc. If the annotations are rendered within the XYChart then they are not clickable, even with z-index changes. This means we need to render them above and create our own annotations overlay which handles synchronising the chart dimensions and scales for correct positioning of the annotations.
def84b6
to
3e33514
Compare
bugbot run |
3e33514
to
7176c2d
Compare
…fying label positioning logic and improving code clarity.
…dering in LineChart. Update renderLabel methods to utilize these new components, enhancing code clarity and maintainability.
…ing proper rendering only when scales are stable.
- fallback to centered position for Safari due to foreign object issues - add space around popover to allow for hitting viewport boundaries
…for clarity and update import references accordingly.
…mensions. Update tests to ensure proper behavior when annotations are provided, absent, or empty.
…f interface methods.
e51b142
to
16dd7d4
Compare
Include the breaking change to the annotations API
972efef
to
6616ff8
Compare
Fixes CHARTS-47: Line Chart: Custom annotations
Proposed changes:
Adds support for custom annotations with interactivity
renderLabel
prop per annotation item which renders custom HTML instead of a basic labelrenderLabelPopover
prop per annotation which turns the label into a button and will open a popup with custom HTMLScreenshots
Notes
Includes a breaking change to the annotations API, from an
annotations
prop to a compound component pattern, with declarative nestedLineChart.Annotation
components.Due to a known issue with positioning within
foreignObject
elements in Safari, the popovers are positioned in the center of the viewport rather than next to the annotation. This can be improved in a further iteration.The annotations are rendered in a separate layer above the tooltips, so they are focused individually for now. I think in a further iteration we should add handling similar to what we have for tooltips, so that the annotations overlay is one stop in the tabindex and annotations are focused within that, using arrow keys.
Other information:
Jetpack product discussion
pcO4xN-2zR-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions: