-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add overflow legend styles to LineChart #1619
Conversation
fbc026f
to
3ac9fec
Compare
size-limit report 📦
|
7803fb5
to
7c677c0
Compare
1a5b3de
to
097dee9
Compare
@@ -70,5 +78,5 @@ export function useColorVisionEvents(enabled = true) { | |||
item.removeEventListener('blur', onMouseLeave); | |||
}); | |||
}; | |||
}, [id, enabled, hiddenIndexes]); | |||
}, [id, enabled, hiddenIndexes, dimensions]); |
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 need the event listeners to be re-added when the dimensions change so they can be applied to the updated displayed legend items.
enableHideOverflow?: boolean; | ||
/* Width is required if enableHideOverflow is true */ | ||
width?: number; | ||
renderHiddenLegendLabel?: RenderHiddenLegendLabel; |
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 added this prop as a callback so we can pass in i18n-ized text from web.
85025a7
to
2cf66e8
Compare
93af03e
to
3e62f75
Compare
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.
Amazing work! 🎉
Code and 🎩 looks great! Confirmed overflow legend behaviour for Line
,VerticalBar
, and StackedArea
charts 👍
@@ -46,20 +76,27 @@ export function LegendItem({ | |||
index, | |||
}); | |||
|
|||
const background = backgroundColor ?? selectedTheme.legend.backgroundColor; |
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.
Curious why background color is being passed in now?
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 the hidden legend tooltip, I wanted to override the theme's legend background color with transparent
so that it matches the standard tooltip styles 😅
const defaultPosition = useMemo( | ||
() => ({ | ||
top: 0, | ||
left: 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.
I see the existing TooltipWrapper
using a position interface that looks something like:
TooltipPosition {
x: number;
y: number;
...
Curious, any reason you went with top
and left
here?
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 TooltipAnimatedContainer
uses the css transform
property since its more performant to animate translations than positions. Since my tooltip is not animated, I just used css position
properties instead.
renderHiddenLegendLabel={renderHiddenLegendLabel} | ||
width={width} | ||
dimensions={dimensions} | ||
enableHideOverflow |
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 talked about enabling this as the default, however do we still want to allow the option for a consumer to override the default? i.e make enableHideOverflow
a prop on the 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.
Good question, I'll check with UX!
… StackedAreaChart
55f0bdf
to
fba7688
Compare
What does this implement/fix?
Hide overflowing legend items in a tooltip for horizontal legends.
In this PR:
enableHideOverflow
prop toLegendContainer
. Enable by default forLineChart
,VerticalBarChart
, andStackedAreaChart
.HiddenLegendTooltip
component to display hidden legend items. I originally tried to refactorTooltipWrapper
/TooltipAnimatedContainer
for this purpose, but creating a separate tooltip seemed to be more appropriate since it's a very different use case. I think there is potential to revisit this in the future.Not in this PR:
Hover events for legend items inI added this in the second commit.HiddenLegendTooltip
. This PR was already getting pretty big so before I make any considerable changes touseColorVisionEvents
, I wanted to merge this first.Does this close any currently open issues?
Resolve https://github.com/Shopify/core-issues/issues/64096
What do the changes look like?
Figma specs
The overflow rules are:
100px
In general, we should try to show as many items as possible.
Storybook link
Long Legend story
Before merging
Check your changes on a variety of browsers and devices.
Update the Changelog's Unreleased section with your changes.
Update relevant documentation, tests, and Storybook.
Make sure you're exporting any new shared Components, Types and Utilities from the top level index file of the package