Shopify / polaris-viz

A collection of React and React native components that compose Shopify's data visualization system
https://polaris-viz.shopify.dev
Other
335 stars 26 forks source link

Check chart bounds when mouse enters chart #1743

Closed envex closed 1 month ago

envex commented 1 month ago

What does this implement/fix?

We were only updating the dimensions on resize, so dimensions.x and dimensions.y were outdated when we were trying to calculate the tooltip position here.

In this PR, we added a onMouseEnter and onFocusIn handler to also update the dimensions when you interact with the chart.

Does this close any currently open issues?

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

What do the changes look like?

Before: https://github.com/user-attachments/assets/7f881726-54ef-45c1-aa6d-3a8634ced5d0

After: https://github.com/user-attachments/assets/232f1c24-0355-4d1d-bfd0-8c30a6c47516

Storybook link

Toggle the top/left margins and make sure the tooltips are in the correct position. Test with scrolling as well.

https://6062ad4a2d14cd0021539c1b-whnpeerjyh.chromatic.com/?path=/story/polaris-viz-charts-barchart-playground--external-tooltip-with-frame

Before merging

github-actions[bot] commented 1 month ago

size-limit report πŸ“¦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.55 KB (0%) 1.3 s (0%) 767 ms (+7.72% πŸ”Ί) 2 s
polaris-viz-cjs 216.43 KB (+0.05% πŸ”Ί) 4.4 s (+0.05% πŸ”Ί) 1.8 s (-5.86% πŸ”½) 6.1 s
polaris-viz-esm 177.03 KB (+0.05% πŸ”Ί) 3.6 s (+0.05% πŸ”Ί) 1.4 s (-4.22% πŸ”½) 4.9 s
polaris-viz-css 4.73 KB (0%) 95 ms (0%) 330 ms (-19.79% πŸ”½) 424 ms
polaris-viz-esnext 182.7 KB (+0.05% πŸ”Ί) 3.7 s (+0.05% πŸ”Ί) 1.2 s (-9.42% πŸ”½) 4.9 s
susiekims commented 1 month ago

Can you just explain why we are making this change to ChartContainer and it now needs to care about recalculating the chart bounds on mouse enter, when it never did before? Just want to ensure this fixes the original problem. Should any of our previous code to handle portals be removed?

@carysmills Yes definitely! This bug probably existed since we started using portals to render tooltips. Prior to using portals, ChartDimensions was only concerned with the chart's height and width. I see in the first version of ChartDimensions, we were only storing height and width in state, not the x or y position.

When we switched to using portals, we needed the chart's position to correctly derive the tooltip position relative to the page. So, we began storing the chart's x and y position in state. Since we only cared about the height/width before, that state was only being updated on resize.

To fix the bug, rather than updating the state anytime the chart x/y changes, we opted to update it on demand (when the user mouses/focuses in chart) to reduce the amount of re-renders.

Does anything else need to happen to fix the area chart tooltip issue, or is this it?

I think this + the change I made in web in my other PR should fix the area chart bugs.