feat(storybook): custom decorators for chromatic coverage #2379

mdt2 · 2023-12-18T21:28:45Z

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

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

Background color @jenndiaz

When default bg color is active, the background is Light
When default bg color is active, the background color can be changed in the control panel
When stacked bg colors is active, the background color cannot be changed in the control panel (it shouldn't even show as a control)
When stacked bg colors is active, the component shows three times with our three different background colors, stacked one on top of the other
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)
When side-by-side bg colors is active, the component shows three times with our three different background colors, side by side
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

When medium - desktop is active, the component shows at the medium platform size
When medium - desktop is active, the platform size can be changed in the control panel
When large - mobile is active, the component shows at the large platform size (usually a bit bigger visually)
When large - mobile is active, the platform size cannot be changed in the control panel (it shouldn't even show as a control)
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)
When medium + large is active, the platform size cannot be changed in the control panel (it shouldn't even show as a control)
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:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

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

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. ✨

github-actions · 2023-12-18T21:54:06Z

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

github-actions · 2023-12-18T21:56:06Z

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.

jenndiaz

I got through BG colors and text direction, will plan to continue tomorrow.

Really excited for this! I think it will help a lot to have this coverage.

jenndiaz · 2023-12-19T01:16:47Z

.storybook/decorators/index.js

+
+		if (showBothScales) {
+			return html`
+				<div style="margin-bottom: 1rem" class=${mediumClass}>


When displaying both scales on component where a popover is displayed, this space is not enough to also display the associated popover or causes some weirdness with how the components are displayed.

Date Picker

Also, Alert Dialog, Combobox and Popover

jenndiaz · 2023-12-19T01:21:28Z

.storybook/decorators/index.js

+    // prep the display areas with the correct classes
+    const isExpress = args.express;
+
+    const backgroundColorClassNames = ['spectrum--light', 'spectrum--dark', 'spectrum--darkest'];


will including darkest cause issues when we move to s2 and no longer support darkest?

Good callout! I can probably make this dynamic.

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)

jenndiaz · 2023-12-19T15:42:35Z

.storybook/decorators/index.js

+		const DIRECTIONS = {
+			left: 'ltr',
+			right: 'rtl',
+			both: 'both',


displaying both directions does cause issues with modal components, (modal, dialog, or alert dialog). For alert dialog I observed it displaying both buttons, but only the rtl modal would appear regardless of the button used.

Are we okay with this being a known issue?
Is there a way to disable buttons for certain components?

jenndiaz · 2023-12-19T15:43:52Z

.storybook/decorators/index.js

+		const SCALES = {
+			medium: 'medium',
+			large: 'large',
+			both: 'both',


similar both issue here as commented in text direction, modal component display on top of each out.

jenndiaz · 2023-12-19T16:50:55Z

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

mdt2 · 2024-01-04T15:33:51Z

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.

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 - Turn on size decorator so Chromatic tests components at each size in one snapshot

…r settings Kind of a bandaid fix. See comment #2379 (comment). - Add utilities for finding component IDs for fullscreen and popover components - Don't render fullscreen and popover components when multiple/both option from toolbar is selected - Instead, render an error message instructing user to choose a single-setting toolbar option

castastrophe · 2024-01-04T16:13:28Z

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: #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 · 2024-01-04T16:18:28Z

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

mdt2 requested review from jawinn and jenndiaz December 18, 2023 21:29

jenndiaz reviewed Dec 19, 2023

View reviewed changes

mdt2 added 3 commits January 4, 2024 10:37

chore: formatting

849af83

mdt2 force-pushed the mdt2/custom-decorators-chromatic-coverage branch from c650a7c to 3843556 Compare January 4, 2024 15:37

mdt2 mentioned this pull request Jan 5, 2024

feat(storybook): add a shared decorator for group rendering and static color settings #2398

Closed

14 tasks

mdt2 added the do not merge A flag for a branch indicating it should not be merged. label Feb 8, 2024

castastrophe marked this pull request as draft April 1, 2024 14:04

castastrophe closed this Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storybook): custom decorators for chromatic coverage #2379

feat(storybook): custom decorators for chromatic coverage #2379

mdt2 commented Dec 18, 2023 •

edited by jenndiaz

Loading

github-actions bot commented Dec 18, 2023 •

edited

Loading

github-actions bot commented Dec 18, 2023 •

edited

Loading

jenndiaz left a comment

jenndiaz Dec 19, 2023

jenndiaz Dec 19, 2023

mdt2 Dec 19, 2023

jenndiaz left a comment •

edited

Loading

jenndiaz Dec 19, 2023

jenndiaz Dec 19, 2023

jenndiaz commented Dec 19, 2023

mdt2 commented Jan 4, 2024

castastrophe commented Jan 4, 2024

castastrophe commented Jan 4, 2024

feat(storybook): custom decorators for chromatic coverage #2379

feat(storybook): custom decorators for chromatic coverage #2379

Conversation

mdt2 commented Dec 18, 2023 • edited by jenndiaz Loading

Description

How and where has this been tested?

Validation steps

Regression testing

To-do list

github-actions bot commented Dec 18, 2023 • edited Loading

github-actions bot commented Dec 18, 2023 • edited Loading

File metrics

Summary

jenndiaz left a comment

Choose a reason for hiding this comment

jenndiaz Dec 19, 2023

Choose a reason for hiding this comment

jenndiaz Dec 19, 2023

Choose a reason for hiding this comment

mdt2 Dec 19, 2023

Choose a reason for hiding this comment

jenndiaz left a comment • edited Loading

Choose a reason for hiding this comment

jenndiaz Dec 19, 2023

Choose a reason for hiding this comment

jenndiaz Dec 19, 2023

Choose a reason for hiding this comment

jenndiaz commented Dec 19, 2023

mdt2 commented Jan 4, 2024

castastrophe commented Jan 4, 2024

castastrophe commented Jan 4, 2024

mdt2 commented Dec 18, 2023 •

edited by jenndiaz

Loading

github-actions bot commented Dec 18, 2023 •

edited

Loading

github-actions bot commented Dec 18, 2023 •

edited

Loading

jenndiaz left a comment •

edited

Loading