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 25 forks source link

Add Grid visualization #1720

Closed mollerjorge closed 3 weeks ago

mollerjorge commented 2 months ago

What does this implement?

This PR adds support for a new visualization called Grid. This visualization is going to be used in RFM analysis.

Designs: Link

Screenshot 2024-09-25 at 4 30 34 PM

Storybook link

Link

Before merging

github-actions[bot] commented 2 months ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.6 KB (0%) 1.3 s (0%) 836 ms (-22.59% 🔽) 2.1 s
polaris-viz-cjs 222.03 KB (+2.58% 🔺) 4.5 s (+2.58% 🔺) 2.4 s (+31.96% 🔺) 6.8 s
polaris-viz-esm 180.17 KB (+1.79% 🔺) 3.7 s (+1.79% 🔺) 1.3 s (-5.79% 🔽) 4.9 s
polaris-viz-css 5.46 KB (+15.58% 🔺) 110 ms (+15.58% 🔺) 446 ms (+35.89% 🔺) 555 ms
polaris-viz-esnext 186.82 KB (+2.26% 🔺) 3.8 s (+2.26% 🔺) 1.3 s (-8.47% 🔽) 5.1 s
mollerjorge commented 1 month ago

Overall this looks great! 🔥

I left some suggestions on the animations though. Currently, it looks a bit jarring with everything moving at the same time. Specially if this component is used multiple times on the same page, it can be a bit too much 👀

If we tone down the intensity a bit and stagger the animation of each cell, it looks more harmonious when comparing to the existing animations in the library.

Before

Screen.Recording.2024-10-25.at.10.58.42.AM.mov After

Screen.Recording.2024-10-25.at.11.04.01.AM.mov

Thank you so much for the suggestion and feedback @krystalcampioni (crazy that I got to add this after our interview a couple months ago on the same visualization) 😄

I like that animation better than the one that we currently have, looks smoother!

I'll check with @arushi-singh45 so that I can have her ok too on that as she was the one that suggested fading all in from the center at the same time.

@arushi-singh45 let me know your thoughts on this :)

arushi-singh45 commented 1 month ago

I like the new treatment as well, feels less intense with the rectangles not growing.

Just one nit - I don't think the axis & label should animate. In our other vizzes they just appear on load.

mollerjorge commented 1 month ago
  • It looks like on small screens, the chart isn't respecting the size/padding around the edge:
Screenshot 2024-10-24 at 1 47 57 PM
  • Can we add any accessibility handling to the chart? For example, aria-labels for each section, the ability to tab to each section, etc. Testing with a screen reader and keyboard navigation should help. We should also consider turning off or limiting the animations when the user has reduced motion turned off (you can target this with a media query).
  • @envex and I have been talking about creating an "experimental" directory where new charts could go as we are still working out some of the kinks. The idea is that this repo is open source, so if we think it's quite likely we'll need to make breaking changes to a component in the future then this would help indicate that to other consumers. Related to this - I wonder if we should make the Random Data story actually random or make it have more data just to see how the chart handles different data to what you expect. Not sure if you are expecting very different types of data for your usecase or not

Noted! @carysmills I added the following accessibility wise:

  1. Tab and focus behavior to navigate through group cells
  2. Aria labels for the group cells
  3. On keyboard navigation, the tooltip stays visible.
  4. When user has reduced motion on, I disabled all animations.

Here is a quick video, let me know what you think (not sure why the focus appears black on the video as I'm seeing it as light blue):

https://github.com/user-attachments/assets/11adafbc-51ff-4b84-98ef-9e68d99f3b8a

mollerjorge commented 1 month ago
  • It looks like on small screens, the chart isn't respecting the size/padding around the edge:
Screenshot 2024-10-24 at 1 47 57 PM
  • Can we add any accessibility handling to the chart? For example, aria-labels for each section, the ability to tab to each section, etc. Testing with a screen reader and keyboard navigation should help. We should also consider turning off or limiting the animations when the user has reduced motion turned off (you can target this with a media query).
  • @envex and I have been talking about creating an "experimental" directory where new charts could go as we are still working out some of the kinks. The idea is that this repo is open source, so if we think it's quite likely we'll need to make breaking changes to a component in the future then this would help indicate that to other consumers. Related to this - I wonder if we should make the Random Data story actually random or make it have more data just to see how the chart handles different data to what you expect. Not sure if you are expecting very different types of data for your usecase or not

Noted! @carysmills I added the following accessibility wise:

  1. Tab and focus behavior to navigate through group cells
  2. Aria labels for the group cells
  3. On keyboard navigation, the tooltip stays visible.
  4. When user has reduced motion on, I disabled all animations.

Here is a quick video, let me know what you think (not sure why the focus appears black on the video as I'm seeing it as light blue):

Screen.Recording.2024-10-25.at.4.39.41.PM.mov

Padding issue should be fixed now @carysmills :)

https://github.com/user-attachments/assets/fae33015-6588-4d93-8835-a8938af6e739