-
Notifications
You must be signed in to change notification settings - Fork 54
feat: Chart tooltip & legend #219
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: hunter/polar-charts-2
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
| * Custom component to render bars for this series. | ||
| */ | ||
| BarComponent?: BarComponent; | ||
| }; |
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.
| background = elevated ? 'var(--color-bg)' : 'transparent', | ||
| borderRadius, | ||
| background = elevated ? 'var(--color-bgElevation1)' : 'transparent', | ||
| borderRadius = 4, |
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.
We had some inconsistencies with ChartText/DefaultScrubberBeaconLabel across web and mobile - this fixes them.
| color?: string; | ||
| shape?: LegendShape; | ||
| value: React.ReactNode; | ||
| }; |
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'm gonna take another look through this component to see if we can clean it up.
| * Position of the legend relative to the chart. | ||
| * @default 'bottom' | ||
| */ | ||
| legendPosition?: LegendPosition; |
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.
For legend implementation there are a few paths
Location: inside or outside of svg
Implementation pattern: as a child of chart or as a prop
Material UI uses a slot prop and it is located outside of the SVG, recharts supports it as a child and it is inside of the SVG.
I decided on location being outside of the SVG since we can reuse parts with ChartTooltip on web (same legend shape component) and we can rely on standard flex CSS properties to support wrapping to multiple lines, customization (such as pressable legend items) is accessible, etc.
Downside of this path is that we have to accept it as a prop or do some funky workarounds - ChartTooltip doesn't have this issue as it can use createPortal from react dom.
I tried using portal for legend - I needed 4 Box components always present, one for each position. However createPortal doesn't exijst on mobile and so we would be back to square one, or we could do some sort of iteration on children for chart - nothing seemed perfect here.
As a result I ended up at providing it as a prop. The biggest downside is someone couldn't add multiple legends but I don't think that is a deal breaker.

What changed? Why?
This PR adds legend and tooltip components. ChartTooltip is only available on web but legend is available on both platforms. I named it this way to not interfere with regular Tooltip, we took a similar approach with 'ChartText'.
There are some items that I am implementing in a different branch as it will require some refactoring
Root cause (required for bugfixes)
UI changes
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false