-
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 tooltips to Funnel Chart Next #1791
Conversation
b363a3a
to
29a6655
Compare
size-limit report 📦
|
29a6655
to
0bfb45e
Compare
packages/polaris-viz/src/components/FunnelChartNext/components/Tooltip/Tooltip.tsx
Outdated
Show resolved
Hide resolved
maxWidth={TOOLTIP_WIDTH} | ||
theme={DEFAULT_THEME_NAME} | ||
> | ||
{() => ( |
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.
Is this function needed 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.
This is because the TooltipContentContainer
accepts the children prop as a function to pass the activeColorVisionIndex
to it's child components, however in the funnel chart tooltips we don't need the color vision functionality as the tooltip uses fixed colors.
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.
Left some minor comments but overall looks great.
I think for open funnels we won't be showing any tooltips, so maybe its worth adding a prop to toggle between showing/hiding tooltips.
2fb7241
to
8bac9ac
Compare
8bac9ac
to
7ae7777
Compare
What does this implement/fix?
This PR adds back the funnel chart tooltips removed in #1785, and makes some changes to when we show the
dropped off
labels. The first segment will now only show aReached this step
label, with subsequent tooltips containing thedropped off
data related to the previous segment.Does this close any currently open issues?
What do the changes look like?
Storybook link
https://6062ad4a2d14cd0021539c1b-gqxlmulfcd.chromatic.com/?path=/story/polaris-viz-charts-funnelchartnext--default
showTooltip
prop and ensure it worksBefore 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