feat: expose closeOnSelect prop in S2 MenuItem
#8035
Conversation
| @@ -157,6 +157,7 @@ export let menuitem = style({ | |||
| }, | |||
| paddingBottom: '--labelPadding', | |||
| backgroundColor: { // TODO: revisit color when I have access to dev mode again | |||
| // @ts-expect-error Expression produces a union type that is too complex to represent | |||
There was a problem hiding this comment.
This will be fixed by chore: Update typescript to 5.8
| <Button aria-label="Closing behavior"><More /></Button> | ||
| <Menu {...args}> | ||
| <MenuItem>I will close the menu by default</MenuItem> | ||
| <MenuItem closeOnSelect={false}>I will not close the menu</MenuItem> |
There was a problem hiding this comment.
It seems odd that some items would close on select but others wouldn't except in different Section Level contexts https://react-spectrum.adobe.com/react-aria/Menu.html#section-level-selection
https://react-spectrum.adobe.com/s2/index.html?path=/docs/menu--docs#selection-groups
Can you explain more about your use case that you need this? Or can you use the Section level selection?
There was a problem hiding this comment.
Yes, so the issue I was hoping to solve here is that I don't want the auto-close behavior to apply when the selection mode is "single," but there's not currently a way (that I could discover) that you can override that behavior on Menu. The use case I have is: I have a Menu wrapped within a Popover, but the menu isn't the only thing within that popover. It's a settings popover with multiple options to select outside of the menu items. When selecting an item from the menu, the popover closes automatically, even if the user is still adjusting settings. We have "apply" and "cancel" buttons for when the user has finished with their selections. When the selection mode is "multiple," the popover stays open while the user is editing the settings. But when it's "single," it auto-closes on selection.
I noticed that the logic was already in place for this closeOnSelect prop to be present, but it's just not exposed on the interface for MenuItem's props, so I figured that exposing it wouldn't be a problem.
I agree that having some items close on select and others not would be confusing -- ideally there would be a top level prop that lives on Menu that could control all the items so it wouldn't need to be specified for each individual item.
There was a problem hiding this comment.
I think a top level Menu prop could be considered with Section level as well. I thought this already existed, but it looks like it was only in the Spectrum 1 implementation, v3. I think it was just missed when implementing RAC and S2. Originally it was on the MenuTrigger, however, I think this should be on Menu/Sections.
Found some more proof it is intended to exist in some form. https://react-spectrum.adobe.com/s2/index.html?path=/docs/migrating--docs#menutrigger
Do you want to update this PR? It should be implemented in RAC first, then exposed to S2.
There was a problem hiding this comment.
Can you explain what being implemented in RAC first means?
There was a problem hiding this comment.
Ah, (almost) all of S2 is built on RAC, our headless component library.
https://react-spectrum.adobe.com/react-aria/Menu.html
So this functionality will need to be exposed there in order to expose it to S2. Doesn't need to be separate PRs or anything, for S2, it may just come for free since types extend RAC as well and our props are usually passed along.
There was a problem hiding this comment.
Oh, React Aria Components, got it. Wasn't familiar with the acronym. I can look into it, but I may not have time right now. The only reason I opened this PR was because it was potentially a quick change, but it looks like it'll take more time to make sure everything works as expected when changing the RAC Menu. I'll close this PR for now, do you want me to open an issue for tracking purposes?
I also added 2
// @ts-expect-errorcomments inMenu.tsxto silence a linting error my IDE was giving me with the style macros. If these should be removed that's fine with me!