spectrum-css icon indicating copy to clipboard operation
spectrum-css copied to clipboard

feat(storybook): custom decorators for chromatic coverage

Open mdt2 opened this issue 2 years ago • 6 comments

Description

Overall, this work will increase our Chromatic coverage without increasing our number of snapshots by showing LTR and RTL, all three background colors, both platform sizes, and every available size of component to Chromatic by default.

  • Implement custom decorators for background color, platform scale, and text direction
  • Turn off controls when custom decorators are in use in the toolbar (because the controls don't work unless the global value in the toolbar is set to "default," so removing them to avoid confusion of which to use)
  • Turn on size decorator so Chromatic tests components at each size in one snapshot

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Text direction @jenndiaz

  • [x] When ltr is active (the default), the component has the ltr direction
  • [x] When rtl is active, the component have the rtl direction
  • [x] When ltr + rtl is active, there are two components showing, one is ltr and the other is rtl

Background color @jenndiaz

  • [x] When default bg color is active, the background is Light
  • [x] When default bg color is active, the background color can be changed in the control panel
  • [x] When stacked bg colors is active, the background color cannot be changed in the control panel (it shouldn't even show as a control)
  • [x] When stacked bg colors is active, the component shows three times with our three different background colors, stacked one on top of the other
  • [x] When side-by-side bg colors is active, the background color cannot be changed in the control panel (it shouldn't even show as a control)
  • [x] When side-by-side bg colors is active, the component shows three times with our three different background colors, side by side
  • [x] If you go back to the default default bg color, the control should show back up in the control panel. If you adjust it, the component adjusts accordingly.

Platform sizes @jenndiaz

  • [x] When medium - desktop is active, the component shows at the medium platform size
  • [x] When medium - desktop is active, the platform size can be changed in the control panel
  • [x] When large - mobile is active, the component shows at the large platform size (usually a bit bigger visually)
  • [x] When large - mobile is active, the platform size cannot be changed in the control panel (it shouldn't even show as a control)
  • [x] When medium + large is active, two components show, with a heading of "Platform Size: Size", and one is medium and the other is large (in alignment with their heading)
  • [x] When medium + large is active, the platform size cannot be changed in the control panel (it shouldn't even show as a control)
  • [x] If you go back to the default medium - desktop, the control should show back up in the control panel. If you adjust it, the component adjusts accordingly.

Testing together! Try some combinations of the controls:

  • [ ] Set ltr + rtl, side-by-side bg colors, and medium + large to see what Chromatic will see
  • [ ] Try all sorts of combinations. Try to break it. Adjust control panel controls and then their toolbar counterparts, etc.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • [ ] The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • [ ] VRTs have been run and looked at.
  • [ ] Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

  • [x] I have read the contribution guidelines.
  • [ ] I have updated relevant storybook stories and templates.
  • [ ] I have tested these changes in Windows High Contrast mode.
  • [ ] If my change impacts other components, I have tested to make sure they don't break.
  • [ ] If my change impacts documentation, I have updated the documentation accordingly.
  • [ ] ✨ This pull request is ready to merge. ✨

mdt2 avatar Dec 18 '23 21:12 mdt2

🚀 Deployed on https://pr-2379--spectrum-css.netlify.app

github-actions[bot] avatar Dec 18 '23 21:12 github-actions[bot]

File metrics

Summary

Total size: 3.97 MB*

🎉 No changes detected in any packages

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions[bot] avatar Dec 18 '23 21:12 github-actions[bot]

I am also seeing some issues with text colors not adjusting for color theme when displaying multiple background colors.

Well: if you toggle the individual background layers in controls the font adjusts correctly but if you display all 3 the font is displaying incorrectly on dark and darkest.

Same thing with:

  • Radio read only variant
  • Treeview with section, the section headers

jenndiaz avatar Dec 19 '23 16:12 jenndiaz

With everything on, ltr + rtl, stacked, and medium + large, I was surprised how well everything displayed.

The popover/modal/is-open components consistently do not display well with any of the multiple or both. (Alert Dialog, Alert Banner, CoachMark, Color Loupe, ComboBox, Contextual help, Datepicker, Dialog, Menu: tray submenu, Modal, Picker, Popover, Search within, Tray, Underlay)

Good catch. I've been researching and trying different things to fix this. Here's what I'm finding:

  • Unfortunately, toolbar items can't be added/removed based on the page yet (see also a duplicate, related issue). This would've been my ideal solution, to just turn off the multiple/both option for popover and modal components. Sad.
    • We did find a plugin that could help with conditional toolbar items, but the last update was three years ago, and that's a bit too long for it to really be considered maintained.
  • Then I thought, what if we don't use the toolbar for this and instead we can have dynamic argTypes in the control panel. But I found out that argTypes are immutable, so that won't work unless we were to set the control on every story 😬 An option I suppose, but kinda icky.

So I'm about to push up a bandaid fix with logic that says if the component is a popover/fullscreen component, instead of rendering the component, we render an error that the feature is unavailable for that component. Not my favorite...

Jenn and I also chatted a bit about how if popovers were static, the issues around them in this context would be more manageable. But making them static means losing the example of interaction and animation that are CSS based. It's almost like we need an extra story for each popover instance that shows it statically to be able to show a bunch of them on a single page for Chromatic. Which makes me wonder more about a Chromatic-only Storybook instance like react-spectrum has.

Our hope, or at least my hope, with using custom decorators was that it wouldn't be much to manage moving forward compared to a separate Storybook instance for Chromatic, but the way things are heading I wonder if the management level might be more similar than initially expected.

mdt2 avatar Jan 04 '24 15:01 mdt2

This is some really fantastic work! I'm sorry I didn't realize it was going on. One thing I wanted to note is that in the rework of the build systems and architecture that I've been doing, they each include pulling out decorators into distinct files to make maintaining them easier (it also gives us a chance to publish them separately at some point for the greater Storybook community to leverage). See this PR for an example: https://github.com/adobe/spectrum-css/pull/2398

One thing I've done in that PR is tried to create a utility for styling the wrapper element to facilitate groupings for Chromatic - I did this by setting flex box layout on the storybook wrappers but allowing components to opt-out or alter it using a customStorybookStyles object in the args of the story. Let me know what you think of this approach. It might help us handle grouped layouts consistently and allow for dynamically setting the background colors (it also ensures all components contain padding for focus/hover state capturing). My big question mark here is if flexbox is too wonky a layout to use as a default (it can cause weird side effects on components that are designed for use in block for example that might not be super clear until you dig into the styles above the component).

castastrophe avatar Jan 04 '24 16:01 castastrophe

Here's some more readable documentation on the wrapper styles approach I mentioned above: https://github.com/adobe/spectrum-css/pull/2398/files?short_path=de90dbe#diff-de90dbe4186cda8c8dfb22049ae90250d47d1d38c8b61f151d9787cf49186b6b

castastrophe avatar Jan 04 '24 16:01 castastrophe