Skip to content
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 tooltip support for Donut Chart #1587

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelnesen
Copy link
Collaborator

@michaelnesen michaelnesen commented Sep 19, 2023

What does this implement/fix?

Adds tooltip support for DonutChart

Does this close any currently open issues?

What do the changes look like?

Screenshot 2023-09-19 at 10 01 03 AM

Storybook link

http://localhost:6006/?path=/story/polaris-viz-charts-donutchart--tooltip

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

@github-actions
Copy link

@github-actions
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.43 KB (0%) 1.3 s (0%) 1.1 s (-18.03% 🔽) 2.4 s
polaris-viz-cjs 211.62 KB (+0.19% 🔺) 4.3 s (+0.19% 🔺) 3.1 s (+13.88% 🔺) 7.3 s
polaris-viz-esm 173.79 KB (+0.16% 🔺) 3.5 s (+0.16% 🔺) 2.3 s (+12.11% 🔺) 5.8 s
polaris-viz-css 4.56 KB (0%) 92 ms (0%) 426 ms (-23.4% 🔽) 517 ms
polaris-viz-esnext 178.93 KB (+0.16% 🔺) 3.6 s (+0.16% 🔺) 1.5 s (-41.32% 🔽) 5.1 s

@@ -65,6 +71,7 @@ export function DonutChart(props: DonutChartProps) {
legendPosition={legendPosition}
renderInnerValueContent={renderInnerValueContent}
renderLegendContent={renderLegendContent}
renderTooltipContent={tooltipOptions ? renderTooltip : undefined}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, tooltipOptions is what enables the tooltip and defines its options in the donut chart. I believe we want the tooltip to be an opt-in experience cc @rachelng. But perhaps another prop like showTooltip would make more sense? Interested to see what others think

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the showTooltip prop. If we decide to go that route, we should add it to all the other components that can render tooltips.

@envex
Copy link
Collaborator

envex commented Sep 19, 2023

@michaelnesen Tooltips don't show when tabbing through the chart, is that intended?

@michaelnesen
Copy link
Collaborator Author

michaelnesen commented Sep 19, 2023

@michaelnesen Tooltips don't show when tabbing through the chart, is that intended?

@envex Hmm good question, It looks like for all other charts tabbing through data points shows the tooltip. However with the donut chart it's a little different as tabbing through already selects/highlights the legend. I can check with UX to see if they have an opinion.

However I'm currently not sure why the tooltip isn't showing up when the active index changes by focus 🤔 But if all other charts have this tooltip behaviour perhaps it makes sense to also have it in the donut chart for consistency

donut-chart-tabbing.mp4

@michaelnesen michaelnesen marked this pull request as draft September 25, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants