feat(menu): spectrum 2 migration #3686

5t3ph · 2025-04-30T22:07:02Z

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:

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.

Screenshots

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

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

…ctrum-css into seckles/css-614-s2-menu

changeset-bot · 2025-04-30T22:07:06Z

🦋 Changeset detected

Latest commit: a8357a3

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

github-actions · 2025-04-30T22:12:32Z

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

github-actions · 2025-04-30T22:17:19Z

File metrics

Summary

Total size: 1.38 MB^*

Package	Size	Minified	Gzipped
menu	47.05 KB	44.81 KB	5.03 KB

@@ -25,12 +25,12 @@
 .spectrum-ColorHandle {
 	/* outer border as box shadow on the colorhandle */
 	/* TODO replace --spectrum-black-rgb with color-handle-outer-border-color when supported by RGBA */
-	--spectrum-colorhandle-outer-border-color: rgba(var(--spectrum-black-rgb), var(--spectrum-color-handle-outer-border-opacity));
+	--spectrum-colorhandle-outer-border-color: rgb(var(--spectrum-black-rgb), var(--spectrum-color-handle-outer-border-opacity));


These are being changed by the updated Stylelint linter process on commit, so my attempt to change it back didn't stick

@5t3ph use --no-verify at the end of your commit message. That ignores the changes. This happened to me once before

☝️ Try this (Aziz's suggestion), hopefully that resolves it!

rise-erpelding

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.

rise-erpelding · 2025-05-09T17:59:41Z

components/colorhandle/index.css

@@ -25,12 +25,12 @@
 .spectrum-ColorHandle {
 	/* outer border as box shadow on the colorhandle */
 	/* TODO replace --spectrum-black-rgb with color-handle-outer-border-color when supported by RGBA */
-	--spectrum-colorhandle-outer-border-color: rgba(var(--spectrum-black-rgb), var(--spectrum-color-handle-outer-border-opacity));
+	--spectrum-colorhandle-outer-border-color: rgb(var(--spectrum-black-rgb), var(--spectrum-color-handle-outer-border-opacity));


☝️ Try this (Aziz's suggestion), hopefully that resolves it!

rise-erpelding · 2025-05-09T18:01:23Z

.changeset/strong-pianos-work.md

+
+In addition to other small token and minor style changes, there were several new features that were added to this version of menu, including:
+
+- A thumbnail can now be added instead of an icon


Sort of a nit, but initially I read this and interpreted it as "now we use thumbnails instead of icons" which obviously isn't how it works. I get what you mean now though and would leave the decision to clarify up to you.

Silly English language.

rise-erpelding · 2025-05-09T18:06:03Z

.changeset/strong-pianos-work.md

+
+- A thumbnail can now be added instead of an icon
+- A section description can now be included below the menu section heading
+- The actions area previously held action switches for multi-select, and in this version, an external link action icon can be included in that area


This is as good a spot as any for me to say that I noticed in Figma that now we have a couple of other new things that I swear I've never seen before in this spec (looking at S2 / Desktop), so I don't know when they were added, there's this unavailable icon that looks like it would be in the same position as the external link icon, and there's this highlight badge too.

I'd say let's not worry about it in this PR unless you think that's something you can add really quickly (I'm honestly not even sure what we would need to implement the badge), but let's make a follow up ticket and we can address it later on.

rise-erpelding · 2025-05-09T19:16:51Z

components/menu/stories/menu.test.js

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


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.

rise-erpelding · 2025-05-09T19:20:09Z

components/menu/stories/menu.test.js

No notes here for me on this, looks great! 💖

rise-erpelding · 2025-05-09T19:46:30Z

components/menu/index.css

@@ -614,7 +644,6 @@

 .spectrum-Menu-item:focus-visible,
 .spectrum-Menu-back:focus-visible {
-	box-shadow: var(--spectrum-menu-item-focus-indicator-shadow) var(--spectrum-menu-item-focus-indicator-border-width) 0 0 0 var(--spectrum-menu-item-focus-indicator-color-default);
 	outline: var(--spectrum-menu-item-focus-indicator-width) var(--spectrum-menu-item-focus-indicator-outline-style) var(--spectrum-menu-item-focus-indicator-color-default);
 	outline-offset: var(--spectrum-menu-item-focus-indicator-offset);
 	border-radius: var(--spectrum-menu-item-corner-radius);


I wonder if there's a specific reason --spectrum-menu-item-corner-radius doesn't have a mod, maybe it was introduced in Foundations and we accidentally overlooked it? What are your thoughts on adding one here and for .spectrum-Menu-item?

rise-erpelding · 2025-05-09T20:02:05Z

components/menu/index.css

A few things that I couldn't find a place in the code to comment because they're not based on changes you've made here:

I've been wondering about the spacing in the design spec. I noticed that the top edge to icon spacing tokens weren't used (component-top-to-workflow-icon-100 and other sizes). Should we be using them or is that spacing accounted for in a different way? I think right now what you've got for the icon spacing is the padding around the menu item, which is component-top-to-text-100 and its associated sizes, which ends up being just a px or so off at certain sizes.

We're also not using the menu-item-to-items token (for item to item spacing), but there is a 4px margin on the menu items which seems like it's there to accommodate the focus outline? This feels like it might have been something that confused me when I was working on this before; do we need extra space that isn't in the spec to ensure we have enough space for the focus outline? Should we be using the spec on top of that or in place of that?

I don't know if you can see this but the margin-block on the .spectrum-Menu-divider is crossed out, I can't figure out why, but it seems to be doing the right thing anyway. 🤷‍♀️ I don't think this needs to be fixed, I'm just wondering why this is happening.

rise-erpelding · 2025-05-09T20:09:23Z

components/menu/index.css

@@ -465,7 +487,7 @@

 	&:focus,
 	&.is-focused {
-		background-color: var(--highcontrast-menu-item-background-color-focus, var(--mod-menu-item-background-color-key-focus, var(--spectrum-menu-item-background-color-key-focus)));
+		background-color: var(--highcontrast-menu-item-background-color-focus, var(--mod-menu-item-background-color-key-focus, var(--spectrum-menu-item-background-color-keyboard-focus)));


I noticed we're missing a background color on the keyboard focused menu item and I'm wondering if the :focus and :focus-visible states have been being confused for one another? That might need a second look.

I think .is-focus-visible is being applied in the template for the isFocused arg, which feels like it also might be something we've been getting wrong for awhile unless I'm missing something?

One other issue I noticed related to this is that the focused menu items in the testing grid aren't getting the right content colors either (they get the default color instead of the focus color), because I think they might be applied specifically only to :focus and not :focus-visible.

rise-erpelding · 2025-05-09T20:30:16Z

components/menu/index.css

@@ -127,6 +45,16 @@
 	--spectrum-menu-section-header-line-height-cjk: var(--spectrum-cjk-line-height-100);
 	--spectrum-menu-section-header-font-weight: var(--spectrum-bold-font-weight);
 	--spectrum-menu-section-header-color: var(--spectrum-gray-900);


This --spectrum-menu-section-header-color, I think it's supposed to be --spectrum-neutral-content-color-default which ends up aliasing to --spectrum-gray-800.

rise-erpelding · 2025-05-09T20:46:01Z

components/menu/index.css

 	--spectrum-menu-item-checkmark-height: var(--spectrum-menu-item-checkmark-height-medium);
 	--spectrum-menu-item-checkmark-width: var(--spectrum-menu-item-checkmark-width-medium);


It looks like --spectrum-menu-item-checkmark-height-medium, --spectrum-menu-item-checkmark-width-medium, and their associated sizes (small, large, etc.) are tokens, but it doesn't look like they're being used anymore; instead the spec says to use checkmark-icon-size-75 (and other sized friends like -100, -200, etc.).

I noticed that the checkmark is disappearing for the XL size and I'm wondering if that has something to do with it:

5t3ph added 19 commits April 22, 2025 16:53

feat(menu): initial token updates

a3f0d48

feat(menu): move description, thumbnail, linkout styles

5910e48

feat(menu): remove incorrectly added tokens

0004133

chore(tokens): use [email protected]

622a285

feat(menu): resolve styles from tokens package update

2e4a280

feat(menu): thumbnail and linkout markup

a0d1015

chore(*): merge branch 'spectrum-two' of https://github.com/adobe/spe…

658a82b

…ctrum-css into seckles/css-614-s2-menu

feat(menu): background-color and focus-indicator tokens

8c1983c

feat(menu): update workflow icon names

cd7a3c1

feat(menu): incorporate section heading, resolve alignment

d35054a

feat(menu): extend stories to new features

3bcbe9e

feat(menu): update tests with new features

473a44a

feat(menu): alignment improvements

6ecca60

feat(menu): resolve grid template spacing

a904129

chore(*): merge branch 'spectrum-two' of https://github.com/adobe/spe…

8952e1d

…ctrum-css into seckles/css-614-s2-menu

feat(menu): update metadata

336b8bb

chore(*): revert accidental merge changes

7bd21da

feat(menu): cleanup

e46ec98

feat(menu): add back background tokens

83648d9

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 requested a review from rise-erpelding April 30, 2025 22:07

5t3ph mentioned this pull request Apr 30, 2025

feat(menu): s2 migration #2802

Closed

31 tasks

feat(menu): add changelog

ed988bc

castastrophe force-pushed the spectrum-two branch from 2de9dee to ef31bff Compare May 5, 2025 20:42

castastrophe force-pushed the spectrum-two branch 2 times, most recently from 7e8269c to 4ce8d19 Compare May 5, 2025 21:18

aramos-adobe reviewed May 6, 2025

View reviewed changes

components/menu/stories/menu.test.js Show resolved Hide resolved

aramos-adobe reviewed May 6, 2025

View reviewed changes

components/menu/index.css Show resolved Hide resolved

aramos-adobe reviewed May 6, 2025

View reviewed changes

components/menu/stories/template.js Outdated Show resolved Hide resolved

5t3ph added 2 commits May 7, 2025 09:57

feat(menu): resolve truncation examples

df935af

chore(*): merge branch 'spectrum-two' of https://github.com/adobe/spe…

f0541ac

…ctrum-css into seckles/css-614-s2-menu

5t3ph requested a review from aramos-adobe May 7, 2025 15:17

chore(*): resolve metadata updates

a6aa9f4

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

aramos-adobe approved these changes May 8, 2025

View reviewed changes

chore(*): revert merge changes

a8357a3

5t3ph commented May 8, 2025

View reviewed changes

rise-erpelding requested changes May 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(menu): spectrum 2 migration #3686

feat(menu): spectrum 2 migration #3686

5t3ph commented Apr 30, 2025 •

edited by aramos-adobe

Loading

changeset-bot bot commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading

aramos-adobe left a comment

5t3ph May 8, 2025

aramos-adobe May 9, 2025

rise-erpelding May 9, 2025 •

edited

Loading

rise-erpelding left a comment

rise-erpelding May 9, 2025 •

edited

Loading

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025

rise-erpelding May 9, 2025


		In addition to other small token and minor style changes, there were several new features that were added to this version of menu, including:

		- A thumbnail can now be added instead of an icon

		--spectrum-menu-item-checkmark-height: var(--spectrum-menu-item-checkmark-height-medium);
		--spectrum-menu-item-checkmark-width: var(--spectrum-menu-item-checkmark-width-medium);

feat(menu): spectrum 2 migration #3686

Are you sure you want to change the base?

feat(menu): spectrum 2 migration #3686

Conversation

5t3ph commented Apr 30, 2025 • edited by aramos-adobe Loading

Description

S2 Menu references:

New features

Tokens update

New mods

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented Apr 30, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 30, 2025 • edited Loading

github-actions bot commented Apr 30, 2025 • edited Loading

File metrics

Summary

menu

aramos-adobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rise-erpelding May 9, 2025 • edited Loading

Choose a reason for hiding this comment

rise-erpelding left a comment

Choose a reason for hiding this comment

rise-erpelding May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5t3ph commented Apr 30, 2025 •

edited by aramos-adobe

Loading

changeset-bot bot commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading

rise-erpelding May 9, 2025 •

edited

Loading

rise-erpelding May 9, 2025 •

edited

Loading