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

[UA][Polaris-Viz]: Fixed Tooltip Positioning When There Are Many Bars #1620

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

precillieo
Copy link
Contributor

@precillieo precillieo commented Jan 18, 2024

What does this implement/fix?

I fixed the tooltip issue in Vertical BarChart. It incorrectly jumps to the top of the page when there are many bars. Why? getAlteredVerticalBarPosition function function sets its y value to 0, when props.isPerformanceImpacted is true (true when there are more than 60 bars).

I solved this by passing down the dimensions prop in Chart of VerticalBarChart (it determines the position of the container on a page, with scrollY applied), through the TooltipWrapper component to getAlteredVerticalBarPosition, then using its y value when performance is impacted.

Does this close any currently open issues?

Fix https://github.com/Shopify/core-issues/issues/64419

What do the changes look like?

Before After

Storybook link

Storybook link
Chromatic Link

Tophat Instructions

  • Click on the storybook link above.
  • Scroll to the section of the page that says Series Colors Up To Sixteen.
  • Move your mouse across the chart; the tooltips should be on the chart and not jump to the top of the page.

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

Copy link

github-actions bot commented Jan 18, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.35 KB (0%) 1.3 s (0%) 1.2 s (+62.28% 🔺) 2.4 s
polaris-viz-cjs 213.53 KB (+0.04% 🔺) 4.3 s (+0.04% 🔺) 1.7 s (-17.74% 🔽) 6 s
polaris-viz-esm 174.14 KB (+0.05% 🔺) 3.5 s (+0.05% 🔺) 1.3 s (+2.15% 🔺) 4.8 s
polaris-viz-css 4.73 KB (0%) 95 ms (0%) 338 ms (+8.61% 🔺) 433 ms
polaris-viz-esnext 179.51 KB (+0.04% 🔺) 3.6 s (+0.04% 🔺) 1.3 s (-12.44% 🔽) 4.9 s

@precillieo precillieo marked this pull request as ready for review January 18, 2024 09:34
@blboyle
Copy link
Contributor

blboyle commented Jan 18, 2024

@precillieo can you describe the steps needed to tophat this? Thanks!

@precillieo
Copy link
Contributor Author

@precillieo can you describe the steps needed to tophat this? Thanks!

I updated the PR description! Thank you for pointing it out🙏

Copy link
Contributor

@blboyle blboyle left a comment

Choose a reason for hiding this comment

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

looks good!

Copy link
Collaborator

@envex envex left a comment

Choose a reason for hiding this comment

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

Can we pull the tooltip above the chart so we're not covering the data?

@@ -79,7 +79,7 @@ export function getAlteredVerticalBarPosition(
}
}
} else {
y = 0;
y = props.dimensions?.y ?? 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also clamp this value so if it's at the top of the page it "sticks" in place?

@precillieo precillieo requested a review from envex January 18, 2024 16:52
@precillieo precillieo merged commit 9316562 into main Jan 18, 2024
6 checks passed
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.

4 participants