-
Notifications
You must be signed in to change notification settings - Fork 207
feat(action-menu): S2 migration [CSS-1160] #4085
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
base: spectrum-two
Are you sure you want to change the base?
feat(action-menu): S2 migration [CSS-1160] #4085
Conversation
📚 Branch previewPR #4085 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-4085/index.html. |
File metricsSummaryTotal size: 1.43 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsactionbutton
actiongroup
actionmenu
menu
popover
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
2729f84
to
779e411
Compare
🦋 Changeset detectedLatest commit: d1c459a The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
635f709
to
242929c
Compare
4baa099
to
eb8cde2
Compare
313ee43
to
3babcfa
Compare
4d618da
to
f3fa4d4
Compare
f2ef53c
to
409436c
Compare
dbd613f
to
00038a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left more details on some of the popover issues I've been noticing, I'd love to hear your thoughts! I'm cool with these being a follow-up ticket if they can't be solved here.
Otherwise, just some minor adjustments needed, I think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try this and see if you can replicate it, this is what I'm finding that breaks it (regardless of browser):
- Open the Action menu component in Storybook (Docs page or Default story, it doesn't matter, as long as the popover is open, this won't happen if the action menu popover stays closed)
- Open either the Coach mark component or the Popover component in Storybook (Docs page or Default story)
- Expect to see either the height or width of the popover there expand infinitely until you refresh
I think there's something about Action menu, because I can go between Coach mark and Popover just fine.
Another thing I noticed is that if I start on the Popover with position bottom
, then go to Action menu where bottom
isn't a valid option (there's only bottom-start
and bottom-end
), then go back to Popover, the popover ends up in a completely different position. Unsure if that's related or not, but something I noticed.
Unless you've got an idea on how to address it, I'm cool with a follow-up ticket.
41b6453
to
03a7ae4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m still seeing some issues with Action Menu/Popover. I’ve documented most inline and tested across multiple browsers, so hopefully they’re reproducible on your end.
The CSS looks solid overall, though! I wonder if simplifying some of the JS implementation could help stabilize things so we can ship this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still seeing the issue when navigating from an open action menu to something like popover or coachmark without refreshing, both locally and in the PR preview 😕
3f6dad6
to
94e9252
Compare
Dismissing stale review after latest changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this quickly and noted that the popover issues from before are resolved! 🎉 Really, really nice work!
I did check out Coachmark after I checked out Action menu and Popover, and I'm seeing a similar issue there, but only after switching on "has action menu":
It's also visible from the docs page.
94e9252
to
d8fc6e3
Compare
components/popover/index.css
Outdated
opacity: 1; | ||
transition-delay: var(--mod-overlay-animation-duration-opened, var(--spectrum-animation-duration-0, 0ms)); | ||
|
||
@starting-style { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@5t3ph Would you mind validating that I set this up correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is working but partly because the processing is pulling this out of being nested. For future security (in case the preprocessor is changed / browserlist support changes) we should not nest this style. The reason has to do with weirdness with specificity. In case you didn't have it at hand, here's my fave article explaining this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushing up that suggested change now, great point about future-proofing
b6ed5e8
to
14ad6aa
Compare
14ad6aa
to
faeccb5
Compare
faeccb5
to
d1c459a
Compare
}; | ||
|
||
/** | ||
* Action menus can be positioned in four locals relative to the trigger but <em>only one menu can be triggered at a single time</em>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely read this as four "low-kuls" first, and figured maybe it was supposed to be "low-cals." Is that a typo for "locales?" Does that word have multiple spellings that I need to remember? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha yes, it's a typo for locales! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! Just have a few minor questions. ✨
@@ -0,0 +1,32 @@ | |||
/*! | |||
* Copyright 2024 Adobe. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update this to 2025? ✨
isOpen: { | ||
...isOpen, | ||
name: "Pop-up is open", | ||
description: "When the button triggers a pop-up, this should be true when the pop-up is open.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "...this should be true while the pop-up is open." ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing this as well — do we need to increase the height for that Coachmark example? ✨
else role = "menuitemradio"; | ||
} | ||
|
||
// hasCheckbox = selectionMode == "multiple" && !hasActions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this altogether? ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to leave the comments I had so far! I need to go through the menu and popover changes a little more thoroughly tomorrow!
}, | ||
menuArgs: { | ||
customStyles: { | ||
"--mod-menu-inline-size": "max-content", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may have already left a comment somewhere about this, but instead of using the mod here, is it possible to use a --spectrum
custom property? If we're going to remove mods, would it make sense to not introduce another place where one is being used?
There's a couple more menu mods in the PlaceholderIcons
args as well.
|
||
- Adds wrapper classes: `spectrum-ActionMenu`, `spectrum-ActionMenu-trigger`, `spectrum-ActionMenu-popover`, and `spectrum-ActionMenu-menu`. | ||
- Supports long press triggers and four placements (start/end, top/bottom) via the underlying popover API. | ||
- Design reference: [Figma S2 token specs](https://www.figma.com/design/eoZHKJH9a3LJkHYCGt60Vb/S2-Token-specs?node-id=19758-3424). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually different than the link that's in the action menu stories! 🤔
|
||
Updates `@spectrum-css/menu` styles to align with latest Spectrum 2 design specifications and improve accessibility. | ||
|
||
- Added this not to prevent clash with the `.is-selectable` placement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we want to elaborate more on what was added? Does the "this" refer to that extra Wait! I got it- there's a not
selector I saw you added. At the very least, can you add backticks around not
just to set it off from the rest of the sentence? I completely read it wrong, and I'd love to see this item look more like how you wrote the action group changes (specifically that one with where
)
|
||
&:disabled, | ||
&.is-disabled { | ||
/* ideal when we want to disable the button but still allow it's content to be focused */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but it should be "its" instead of "it's" with an apostrophe.
name: "Has popup", | ||
description: "If the button triggers a popup action, this should be set to reflect the type of element that pops-up.", | ||
name: "Has pop-up", | ||
description: "If the button triggers a pop-up action, this should be set to reflect the type of element that pops-up.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that pops-up
shouldn't be hyphenated. Not a blocker though, and not something you introduced either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked with Jess and she confirmed it is "pop-up" when talking about the component but "pop up" when describing the action. So it's probably hyphenated in the name and maybe not supposed to be hyphenated in the description ("a pop up action" vs. a "pop-up component").
[`${rootClass}--static${capitalize(staticColor)}`]: | ||
typeof staticColor !== "undefined", | ||
["is-disabled"]: isDisabled, | ||
["is-hover"]: isHovered, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we missing the ["is-selected"]: isSelected,
class here? I noticed it's not in this list anymore, but does it need to be any longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd removed it in favor of attaching the styles to the aria-pressed state in the CSS but kept the styles for the class as a polyfill for those not using the correct aria yet.
["is-active"]: isActive, | ||
...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}), | ||
})} | ||
[rootClass]: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but we could clean up the identation of the classMap
if we wanted. That might be why there's such a big diff here to begin with... 🤔
type: { summary: "string" }, | ||
category: "Content", | ||
type: { summary: "boolean" }, | ||
category: "Accessibility", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooooh I love the idea of having an accessibility section even in storybook! I have to keep it in mind as we get second-gen SWC up and going and iterating.
...(menuArgs?.customClasses ?? []), | ||
], | ||
customStyles: { | ||
"--mod-menu-inline-size": "100%", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here- do we want to introduce another instance of a mod if we plan to start removing them?
...popoverArgs | ||
} = {}, context = {}) => { | ||
return Popover({ | ||
size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I wonder why we had a sized popover! thanks for catching and removing it!
It looks like maybe we were trying to pass size
to the action button and menu. but as long as we include a size in the menuArgs
and triggerArgs
, consumers can still control the size, correct? I'll be honest that I haven't tried this, but that's how I'm understanding it.
Description
This PR:
@spectrum-css/actionmenu
component that composesActionButton
,Popover
, andMenu
to present action lists from a trigger.@spectrum-css/actionbutton
and@spectrum-css/actiongroup
to treat selection via.is-selected
as well as:where([aria-pressed="true"], [aria-expanded="true"])
to cover more accessibility use-cases while keeping selector specificity low.@spectrum-css/menu
to align with Spectrum 2 specifications and accessibility improvements.Design references:
Includes a changeset with the following bumps:
How and where has this been tested?
Action menu
stories (default, long-press, placements).Menu
stories for focus indicators, CJK line-height, external link/drill-in icons, thumbnails, and forced-colors behavior.Action button
andAction group
selected visuals using.is-selected
,[aria-pressed="true"]
, and[aria-expanded="true"]
.Validation steps
isOpen
and confirm popover/menu spacing and placement reflect updates..is-selected
, setaria-pressed="true"
, andaria-expanded="true"
; confirm identical visuals due to:where(...)
..is-selected
and ARIA attributes.Screenshots
To-do list
Notes for reviewers
:where([aria-pressed],[aria-expanded])
to avoid specificity issues and broaden accessibility support.References
https://github.com/changesets/changesets
https://www.figma.com/design/eoZHKJH9a3LJkHYCGt60Vb/S2-Token-specs?node-id=19758-3424