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

Remove chart level getPosition and getAlteredPosition #1755

Closed envex closed 1 week ago

envex commented 1 week ago

What does this implement/fix?

In order to allow charts to position the tooltip differently, we passed getPosition and getAlteredPosition functions to TooltipWrapper.

Following this logic was confusing, so now we're going to pass the chartType to TooltipWrapper and handle all the different chart logic beside the rendering of the tooltip.

This should allow us to further improve the tooltip logic in smaller PRs.

Storybook link

There should be no visual changes to the charts and tooltips should behave the way the previously behaved.

https://6062ad4a2d14cd0021539c1b-iplfayxntw.chromatic.com/

Before merging

github-actions[bot] commented 1 week ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.64 KB (+0.03% 🔺) 1.3 s (+0.03% 🔺) 1.1 s (+31.29% 🔺) 2.3 s
polaris-viz-cjs 223.6 KB (+0.18% 🔺) 4.5 s (+0.18% 🔺) 2.1 s (+9.8% 🔺) 6.5 s
polaris-viz-esm 180.89 KB (-0.07% 🔽) 3.7 s (-0.07% 🔽) 1.6 s (-4.31% 🔽) 5.2 s
polaris-viz-css 5.55 KB (-0.06% 🔽) 112 ms (-0.06% 🔽) 263 ms (-12.72% 🔽) 374 ms
polaris-viz-esnext 187.64 KB (-0.08% 🔽) 3.8 s (-0.08% 🔽) 1.7 s (+24.49% 🔺) 5.5 s
envex commented 1 week ago

Looks like the iframe is throwing off the calculations - I'll dig in

envex commented 1 week ago

@carysmills I think that issue is fixed in: https://github.com/Shopify/polaris-viz/pull/1756

Do you think it makes sense to fix in this PR as well, or review the other PR so we can merge it into this one?

carysmills commented 1 week ago

@envex I won't be able to review the other one, but I will take a look to see if the issue is fixed on that PR

carysmills commented 1 week ago

@envex I think you missed this part of my review:

I also think the positioning of the line chart tooltip on the "Available charts" page is still off

envex commented 1 week ago

@carysmills Ok - all good to go again