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
Fix controlled menuitems #4051
Fix controlled menuitems #4051
Conversation
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Tachometer resultsChromeaction-menu permalink
picker permalink
split-button permalink
Firefoxaction-menu permalink
picker permalink
split-button permalink
|
Could similar be achieved by moving the application of
Types are not so happy about it at current, but you can seemingly do this declarative with:
|
I'm not sure what the contract is exactly between If so then 👍 that's the way to go. |
No idea whether there are tests that behave differently or not on this. |
It looks like there are tests in menu for when |
It causes this test to fail:
So far I've only come up with fixes that have a breaking API change: either requiring |
What happens if you change https://github.com/adobe/spectrum-web-components/blob/main/packages/menu/src/Menu.ts#L95 to be if (selected === this.selected || !this.selects) {`? There's also likely a reasonable path to retiring a test or two if we need to in order to have an understandable API for this.... |
@Westbrook that also fails the "allows top-level selection state to change" since right now, setting Maybe a new value like The alternative is the inverse, like |
Just to add my 2 cents here, as I also try to extend current ActionMenu to a CustomActionMenu for different reasons, and the selection behavior was one problem to solve here, as we want to control the selection states of the menu-items from the outside.
|
Would be better to confirm that selection is appropriately managed when using |
Description
"A" (probably not "the") fix for the currently-broken controlled action-menu items:
(opening and closing, without clicking on any items, will clear all items' selected states)
(it probably should not trigger the "action" popup immediately; that may be a separate issue)
The root cause is that
PickerBase
always assigns its child<sp-menu>
's.selected
array. SinceActionMenu
extendsPickerBase
, it also always assigns the selected array. So an action menu like this:...will render correctly, once (all items visibly selected), but upon opening & closing the menu, the action-menu will reset the
.selected
array to[]
which will clear the selected state from every item. This behavior started here:#3669
This demonstration fix sidesteps the issue by not setting
selected
onunmanaged
action menus. That change can't be applied directly toPickerBase
because then there are side effects for other components that inherit fromPickerBase
, and even for action menus that are intended to be used by assigning values to.selected
.Sharing this imperfect/hacky fix in order to start a conversation about what a long-term fix could look like.