Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(menu): improve chromatic coverage #2757

Merged
merged 1 commit into from
May 23, 2024

Conversation

rise-erpelding
Copy link
Collaborator

@rise-erpelding rise-erpelding commented May 14, 2024

Description

Previously, there were 13 different variants for menu, and the only component-level controls were for size, max inline-size, and truncation. This work eliminates some variants (down to 5 from 13) in favor of showing multiple menu variants in a the Menu with Variants menu and adding more controls to show various states and variants, which are displayed individually in the Chromatic testing preview.

Single menu item refactor

It was also previously difficult to understand the different options available for a menu item within the menu, in both Storybook and the code. Here, the code has been refactored to render each area of a menu item separately, potentially making the code easier to understand and change as we migrate to Spectrum 2. A story specifically dedicated to display a single menu item has been created, with controls to display combinations of variants relevant to a single menu item (such as displaying the icon, item selected status, whether switches or checkboxes are used to indicate selected status on a multi-select item, and focus state). This single menu item story also aligns more closely with the S1 design spec and S2 design spec, showing all the variants, making it easier to validate.

Chromatic coverage

Chromatic coverage for the single menu item and menu item with variants stories has been expanded in order to align more closely with the testing views that have recently been added to other components and to make all states and variants visible in a single testing view (the "kitchen sink" testing strategy), reducing the number of snapshots needed for visual regression testing the component.

Other changes

The Collapsible menu story and Tray submenu story have been left almost completely unchanged as these variants differ from most others. These two variants also do not have a different Chromatic testing view, but I'm open to opinions on either of these!

This work also includes one minor bug fix that prevents the overlap of a multi-select checkbox and an icon should they both be present--this is the only CSS change.

CSS-614

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

Check both stories' testing previews - open Storybook to check out the stories for Menu

  • Check the testing preview for both Menu with Variants and Menu Item Only - does this cover all variants?
  • Do controls work for both stories? (Note: switching to/from single to multiple select mode does result in a hooks error, but this should resolve upon reload)
  • Are all variants free from visual issues (no overlapping, wonky placement issues, etc.)?
  • Do the Collapsible and Tray Submenu stories look and behave the same compared to production?

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. (@rise-erpelding - will run these again with final changes)
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes. (@rise-erpelding)

Screenshots

Chromatic testing view for Single Menu Item
image

Default view mode for Menu with Variants menu, includes many different menu item options in a single menu
image

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • 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. ✨

Copy link

changeset-bot bot commented May 14, 2024

🦋 Changeset detected

Latest commit: 1ca8dc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/menu Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented May 14, 2024

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

Copy link
Contributor

github-actions bot commented May 14, 2024

File metrics

Summary

Total size: 4.65 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.

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch 2 times, most recently from 7466cde to ca6bf04 Compare May 14, 2024 15:25
@rise-erpelding rise-erpelding added the run_vrt For use on PRs looking to kick off VRT label May 14, 2024
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch 2 times, most recently from b01b447 to dfe313c Compare May 14, 2024 18:32
@rise-erpelding rise-erpelding marked this pull request as ready for review May 14, 2024 18:42
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch from dfe313c to 7380e7d Compare May 14, 2024 18:42
Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few initial thoughts!

components/menu/index.css Outdated Show resolved Hide resolved
components/menu/stories/menu.stories.js Outdated Show resolved Hide resolved
components/menu/stories/menu.stories.js Show resolved Hide resolved
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Thanks for all your work here to get this organized and capturing more variants.

Two ideas for more things that might be good to have in the VRT:

  • Wrapping text on the section header
  • Also including the icon and the checkmark in/after "Sizes" for the "Menu Item Only" story.

.changeset/silly-ties-attend.md Outdated Show resolved Hide resolved
components/menu/index.css Outdated Show resolved Hide resolved
components/menu/stories/template.js Show resolved Hide resolved
components/menu/stories/menu.stories.js Outdated Show resolved Hide resolved
components/menu/stories/menu.stories.js Outdated Show resolved Hide resolved
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch from 7380e7d to 4040886 Compare May 17, 2024 03:05
@rise-erpelding rise-erpelding removed the run_vrt For use on PRs looking to kick off VRT label May 17, 2024
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch 2 times, most recently from d544c07 to dcd1f77 Compare May 20, 2024 20:11
@rise-erpelding rise-erpelding added the run_vrt For use on PRs looking to kick off VRT label May 20, 2024
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch from dcd1f77 to aa2796c Compare May 20, 2024 20:58
@rise-erpelding rise-erpelding removed the run_vrt For use on PRs looking to kick off VRT label May 20, 2024
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch 4 times, most recently from be40103 to ecf7e11 Compare May 21, 2024 01:44
@rise-erpelding
Copy link
Collaborator Author

Updates since last reviews:

Hide standard with dividers story

  • Hide "Standard with dividers" story that is being used by Picker

Use is-focus-visible class to display focus state

  • Hide controls for active, focused, hovered states (these states are shown in Chromatic, but should use hover/focus/active mouse/keyboard functions to trigger them rather than Storybook controls)
  • Use is-focus-visible class to display focus state, eliminating the previous change made to CSS

Adjust Chromatic testing view

  • Make it more obvious that menu headers are headers within the component rather than being part of the typography decorating the component
  • Add seafoam green color to the typography headings closest to the component to make the distinction between component and decorator clear
  • Truncates text that was supposed to be long enough to truncate but wasn't
  • Add icon and single select checkmark for sizing on the Single menu item Chromatic testing preview so that we can see that the icon sizes scale with the component

Adjust switches and controls

  • "Has dividers" control only displays for Menu with Variants story
  • No "Selection Mode" control for Collapsible menu story or Tray Submenu story
  • Add "Is tray submenu" control for Tray Submenu story (previously this was just floating around by itself as it hadn't been defined)
  • Add boolean control for Value and conditional Single Item Value text control for Value text if boolean is true
  • Add Chromatic testing view to show values

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch 2 times, most recently from ecf7e11 to 2460b61 Compare May 22, 2024 15:19
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great 👍 The requested changes from my last review are all resolved.

@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label May 22, 2024
- refactor menu item

- create single menu item story
- create menu with variants story
- build chromatic testing views for menu item and menu with variants
- remove extra stories in favor of using menu with variants

- fix to prevent multiselect checkbox and icon clash

CSS-614
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-614-menu-refactor branch from 2460b61 to 1ca8dc1 Compare May 23, 2024 16:38
@pfulton pfulton merged commit e945e26 into main May 23, 2024
22 checks passed
@pfulton pfulton deleted the rise-erpelding/css-614-menu-refactor branch May 23, 2024 19:49
@github-actions github-actions bot mentioned this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review ready-to-merge run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants