Skip to content

feat(menu): spectrum 2 migration #3686

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

Open
wants to merge 27 commits into
base: spectrum-two
Choose a base branch
from
Open

Conversation

5t3ph
Copy link
Contributor

@5t3ph 5t3ph commented Apr 30, 2025

Description

First of all, big s/o to @rise-erpelding for the original work in #2802 🙌

S2 Menu references:

New features

  • thumbnail - replaces the icon
  • section header description
  • external link icon - rendered in the actions area
    • Note: There is an outstanding issue to add the correct icon to the system, so the current icon in use is temporary
  • updated icon names for ones that were unavailable in the new set

Also of note is that the grid-template-areas for spectrum-Menu-item were updated to accomodate the positioning of elements relative to the thumbnail.

Tokens update

Since @adobe/spectrum-tokens has been updated to include many tokens relating to the menu component, including some that replace custom tokens that had previously been added. As such, these custom menu item color tokens that are now available from @adobe/spectrum-tokens have been removed.

One special case is --spectrum-menu-item-background-color-default which was spec'd to have "0% opacity" which for a CSS background color translates to transparent so that token is redefined as such. So, menu-item-background-opacity was not used.

Additionally, due to the margin required for the focus indicator previously implemented, the "new" token menu-item-to-item was not implemented as it compounded that margin.

New mods

  • --mod-menu-item-linkout-icon-height
  • --mod-menu-item-linkout-icon-width
  • --mod-menu-item-thumbnail-height
  • --mod-menu-item-thumbnail-opacity-disabled
  • --mod-menu-item-thumbnail-to-label
  • --mod-menu-item-thumbnail-width
  • --mod-menu-item-top-to-thumbnail
  • --mod-menu-section-description-color
  • --mod-menu-section-description-font-size
  • --mod-menu-section-description-font-weight
  • --mod-menu-section-description-line-height
  • --mod-menu-section-description-line-height-cjk
  • --mod-menu-section-header-to-description

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

  • New features added are visible in Storybook and are covered by a Chromatic testing view for one or more stories on the menu component.
    • Thumbnail
    • Section description
    • External link
  • As shown in "Test" mode for "Menu item": menu item elements (icon, thumbnail, checkmarks, checkboxes, switches, external linkout, etc.) all play nicely with each other and nothing appears out of place when combining various elements
    • No selection mode
    • Single select mode
    • Multi-select mode with switches
    • Multi-select mode with checkboxes
    • Drill-in
    • Truncation
    • Text wrapping
  • Menu items and other components within menu behave as expected on hover/active/focus states (for instance, nothing disappears that shouldn't, disabled items don't get hover/active states or pointer cursors, contrast seems appropriate)
    • In regular light/dark contexts
    • In Windows High Contrast Mode
  • New tokens listed in Figma are used - exceptions of menu-item-background-opacity and menu-item-to-item as explained above
    • menu-item-label-to-description size tokens
    • section-header-to-description size tokens
    • menu-top-to-thumbnail size tokens
    • text-to-visual-400 (used for thumbnail to label inline spacing)
    • link-out-icon size tokens
  • Menu matches the design spec
    • Layout - heights and spacings appear to match spec, and corner rounding tokens are in use (Note: Sub-menu alignment with menu item is outside of the scope of Spectrum CSS, I think)
    • Assets - icons, checkboxes, and thumbnails are appropriately sized (Reminder: External link icon needs to be updated to S2 icon when these icons are available to us)
    • Colors - background and content colors use the appropriate color tokens
    • Typography - menu headers and items are using the appropriate typography tokens

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.

Screenshots

image

Note: The desktop guide shows that when thumbnail is active, the selected state check or checkbox should be center aligned

image image image image image

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

@5t3ph 5t3ph added size-5 L ~30-42hrs; lots of effort or complexity, most of a sprint needed to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review S2 Spectrum 2 labels Apr 30, 2025
@5t3ph 5t3ph requested a review from rise-erpelding April 30, 2025 22:07
Copy link

changeset-bot bot commented Apr 30, 2025

🦋 Changeset detected

Latest commit: 417a518

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 Major

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

@5t3ph 5t3ph mentioned this pull request Apr 30, 2025
31 tasks
Copy link
Contributor

github-actions bot commented Apr 30, 2025

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

Copy link
Contributor

github-actions bot commented Apr 30, 2025

File metrics

Summary

Total size: 1.39 MB*
No change in file sizes

Package Size Minified Gzipped
menu 48.33 KB 46.04 KB 5.14 KB
stepper 18.29 KB 17.43 KB 2.39 KB
tag 27.53 KB 26.34 KB 3.08 KB

File change details

menu

Filename Head Minified Gzipped Compared to base
index.css 48.33 KB 46.04 KB 5.14 KB 🔴 ⬆ 6.96 KB
metadata.json 23.81 KB - - 🔴 ⬆ 3.92 KB

stepper

Filename Head Minified Gzipped Compared to base
index.css 18.29 KB 17.43 KB 2.39 KB 🟢 ⬇ 10.24 KB
metadata.json 7.83 KB - - 🟢 ⬇ 6.23 KB

tag

Filename Head Minified Gzipped Compared to base
index.css 27.53 KB 26.34 KB 3.08 KB 🔴 ⬆ 6.67 KB
metadata.json 14.21 KB - - 🔴 ⬆ 3.93 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@castastrophe castastrophe force-pushed the spectrum-two branch 2 times, most recently from 7e8269c to 4ce8d19 Compare May 5, 2025 21:18
@5t3ph 5t3ph requested a review from aramos-adobe May 7, 2025 15:17
@5t3ph 5t3ph added run_vrt For use on PRs looking to kick off VRT and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) labels May 7, 2025
Copy link
Contributor

@aramos-adobe aramos-adobe left a comment

Choose a reason for hiding this comment

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

I see you have some css changes in the color slider and another component that may not be related to your PR. Maybe you want to uncommit those but overall this is amazing work from what I've seen and I learned a ton from it!!

Copy link
Collaborator

@rise-erpelding rise-erpelding 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 really fantastic! I made a lot of comments but not all of them reflect changes that need to be addressed. Here's a small rundown of the changes I think this may still need:

  • The spec calls for the downstate minimum perspective but it isn't used here. Unless we're not doing that for a reason, it probably needs to be added. We're using a decorator to do this. We've implemented it with button and action button just to name a couple of examples, and I think that the documentation we have on this is pretty decent as a reference. Let me know if you need help on this! It is a little flaky and sometimes doesn't work, just to warn you.
  • A couple of little nits on the Storybook controls to make them a little cleaner (and a question about which checkbox variant we want in the template).
  • Another look at the top spacing for the menu item.
  • Another look at the menu item key focus background and content colors, as well as the section heading color.
  • The missing checkmark for the XL menu item needs to be addressed.
  • When I looked at the spec, there were a couple of additions I've never seen before for the highlight badge and unavailable item, I'd recommend writing a follow-up ticket to address those.

value: undefined,
iconName: undefined,
include: ["No selection", "No selection, with description", "Truncation", "Text wrapping"]
}
// {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know what this does? It looks like it adds the .is-highlighted class, does that correspond to ::highlight() (which I totally just looked up and didn't know existed until today)? 🤔

Just curious! Seems like I've overlooked this many times before since it's nothing new to menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see it's being added to the li if it's active, but I don't see the corresponding class in the current CSS, so perhaps a leftover? Maybe to check contrast or check if pointer events for highlighting was disabled?

@5t3ph 5t3ph requested a review from rise-erpelding May 13, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review run_vrt For use on PRs looking to kick off VRT S2 Spectrum 2 size-5 L ~30-42hrs; lots of effort or complexity, most of a sprint needed to complete.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants