feat: chart legend #302
feat: chart legend #302
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
| </Text> | ||
| ) : ( | ||
| label | ||
| )} |
There was a problem hiding this comment.
I am waiting to hear back from design regarding if we should add aria-hidden to the default legend item or not, since users may expect to only hear the name of each series when scrubbing.
| }, | ||
| ]} | ||
| yAxis={{ domain: { min: -domainLimit, max: domainLimit } }} | ||
| aria-hidden="true" |
There was a problem hiding this comment.
This example and above were interfering with screen reader since they automatically update.
| key: 'Combobox', | ||
| getComponent: () => | ||
| require('@coinbase/cds-mobile/alpha/combobox/__stories__/Combobox.stories').default, | ||
| }, |
There was a problem hiding this comment.
This was added when running the sync script
| originY={effectiveOriginY} | ||
| roundBottom={roundBottom} | ||
| roundTop={roundTop} | ||
| seriesId={seriesId} |
There was a problem hiding this comment.
This provides support for custom bar components
| } | ||
| ``` | ||
| You can use `hideBeaconLabels` to hide beacon labels, while still being able to provide a label for a series. |
There was a problem hiding this comment.
This is a nice benefit of legend, teams can still label lines without the beacon labels
|
Tested this with design & confirmed accessibility, made a few changes. It is ready for review again :) |
packages/mobile-visualization/src/chart/legend/DefaultLegendItem.tsx
Outdated
Show resolved
Hide resolved
packages/mobile-visualization/src/chart/legend/DefaultLegendItem.tsx
Outdated
Show resolved
Hide resolved
packages/mobile-visualization/src/chart/legend/DefaultLegendItem.tsx
Outdated
Show resolved
Hide resolved
packages/mobile-visualization/src/chart/legend/DefaultLegendItem.tsx
Outdated
Show resolved
Hide resolved
| * - A React element renders that element as the legend | ||
| * - `false` or omitted hides the legend | ||
| */ | ||
| legend?: boolean | React.ReactElement<LegendProps>; |
There was a problem hiding this comment.
traditionally we have accepted React components instead of JSX elements - what is the rationale here?
There was a problem hiding this comment.
Good catch. Initially I was accepting just a boolean here, and then I saw the need to customize legend without wanting to add too many extra fields to CartesianChart.
I then for a bit was cloning the component passed in to add an additional field but realized this wasn't a great practice so I updated this.
Now it will be boolean | React.ReactNode. However I agree that this still might not be correct. Across the app we accept ReactNode in many places.
SlideButton accepts startUncheckedNode and endCheckedNode and it also accepts SlideButtonHandleComponent and SlideButtonBackgroundComponent.
Select accepts ReactNode for label and also accepts SelectControlComponent.
In these cases we have been accepting the components in cases where we need to pass props, however for here we don't need to since the user can use context. But in the cases where we accept a reactnode the requests for the user are much simpler, maybe an Icon or Text component. Neither of these cases fully match Legend, this feel a little more uncharted.
If we switch to component we could support showLegend and LegendComponent. If we keep reactnode we can do legend and keep with boolean or reactnode.
There was a problem hiding this comment.
@hcopp so the user would need to render something that consumes the chart context under the hood? It almost feels simpler to support the LegendComponent and pass props that way. In that case I would suggest inverting the show to hide: hideLegend
Again, I trust your judgement on this. I think what rang the bell for me was LegendProps - in its current form the customization already forces them to write a component that mirrors our legend interface which could in theory just be passed as a custom component
There was a problem hiding this comment.
The prop is now legend?: boolean | React.ReactNode so we don't need to worry about LegendProps, that was leftover from earlier testing of mine when I was going to see if there was a benefit to cloning the node, and I have dropped that logic entirely.
Just spoke with Adrien. I think this current path is the simplest given the problem doesn't fully sit on either side of our traditional sub/default components or react node.
Someone can make their own Legend fully though and pass in via ReactNode now though!
| * Accessibility label for the legend group. | ||
| * @default 'Legend' | ||
| */ | ||
| legendAccessibilityLabel?: string; |
There was a problem hiding this comment.
when and how do we determine what props we should surface on the parent component? One could argue that the label can be set by providing their own variaiton of the default legend component if the really needed to
There was a problem hiding this comment.
I followed adding this based on Select Alpha's controlAccessibilityLabel. In my testing with design, they expressed the desire for this to be set when using a legend and as such not including this on the base component would require users to always pass in a react node to be compliant.
3 participants
What changed? Why?
This PR adds a new chart component, Legend.
It also adds support for hideBeaconLabels prop to Scrubber and BarComponent to BarChart.
UI changes
Mobile
Web
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