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

Show error state when chart contains infinity #1735

Closed carysmills closed 1 month ago

carysmills commented 1 month ago

What does this implement/fix?

Fixes this broken chart we saw on Sidekick this morning, which results from us passing the chart a value of Infinity

Screenshot 2024-10-10 at 10 25 46 AM

Using the existing error handling in the chart, we can just make the chart show the error state. An error will be thrown, so we should see these errors in bugsnag if the consuming project surfaces them.

Note: I spent a while trying to add a test for this but had issues checking for the error. Please let me know if you have any ideas!

Does this close any currently open issues?

Resolves https://github.com/Shopify/web/issues/144056

What do the changes look like?

Before After
Screenshot 2024-10-10 at 2 59 49 PM Screenshot 2024-10-10 at 2 59 36 PM

Storybook link

You can check the storybook I added: http://localhost:6006/?path=/story/polaris-viz-charts-barchart-playground--infinity-data

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.41 KB (0%) 1.3 s (0%) 993 ms (+25.76% 🔺) 2.3 s
polaris-viz-cjs 215.83 KB (+0.03% 🔺) 4.4 s (+0.03% 🔺) 2 s (+10.65% 🔺) 6.3 s
polaris-viz-esm 176.67 KB (+0.04% 🔺) 3.6 s (+0.04% 🔺) 1.3 s (-9.86% 🔽) 4.8 s
polaris-viz-css 4.72 KB (0%) 95 ms (0%) 334 ms (-34.17% 🔽) 428 ms
polaris-viz-esnext 182.32 KB (+0.03% 🔺) 3.7 s (+0.03% 🔺) 1.2 s (-16.3% 🔽) 4.8 s
carysmills commented 1 month ago

I see the test failure and will check that this doesn't break charts when there's empty data 👀

carysmills commented 1 month ago

Update: I've isolated the change to the vertical bar chart, since that's where the errors are currently happening. I will open an issue to look into this for all the charts, but it will take a bit more digging for the other charts, because we are intentionally setting Infinity internally for some of the other charts.

carysmills commented 1 month ago

@envex yes! I started going down that path, but because we internally set some charts to use Infinity as fallback values in the scale, it got a lil complicated. I have made an issue here to go through all the charts and ensure they don't blow up if Infinity or NaN is passed as a value, since this now seems like a possibility with our new data. I'll book some time with you next week so we can look at this together since you have more recent context on all the charts.