Skip to content
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

Add more menu selection customizability #1189

Closed
adixon-adobe opened this issue Feb 11, 2021 · 8 comments · Fixed by #1668
Closed

Add more menu selection customizability #1189

adixon-adobe opened this issue Feb 11, 2021 · 8 comments · Fixed by #1668

Comments

@adixon-adobe
Copy link
Collaborator

adixon-adobe commented Feb 11, 2021

This is a proposal to better support multiselect and un-selectable menu elements, and allow structured sub-handling in menu groups when necessary. It's modeled after the selects attribute already in sp-action-group. Creating this after a discussion with @Westbrook, and this is intended to consolidated/close issues #350 & #715

sp-action-menu supports new selects attribute

multiselect

With selects="multiple" any number of the items in the menu can be selected, and activating a menu item will toggle it's selected state.

<sp-action-menu selects="multiple">
	<sp-menu-item selected>item 1</sp-menu-item>
	<sp-menu-item>item 2</sp-menu-item>
	<sp-menu-item selected>item 3</sp-menu-item>
</sp-action-menu>

single

With selects="single" zero or 1 item can be selected.

<sp-action-menu selects="single">
	<sp-menu-item selected>item 1</sp-menu-item>
	<sp-menu-item>item 2</sp-menu-item>
	<sp-menu-item>item 3</sp-menu-item>
</sp-action-menu>

no selection (BREAKING CHANGE)

When the selects attribute is ommitted in sp-action-menu, items are not selectable. This better matches the general intent of action menu, where most items are actions that shouldn't be selected (e.g. File -> save... isn't an item you'd expect to be selected)

<sp-action-menu>
	<sp-menu-item>item 1</sp-menu-item>
	<sp-menu-item>item 2</sp-menu-item>
	<sp-menu-item>item 3</sp-menu-item>
</sp-action-menu>

sp-picker & sp-split-button are single-select

There is no selects attribute for these controls, as they're designed to have a single value selected.

sp-menu-group selects attribute

To allow for more granular selection handling, sp-menu-group also can provide a selects attribute, in which case it takes over managing selection for that sub menu. When present, the parent control is not notified of selection changes.

Example 1

This example shows an action menu that has both a multi-select and single-select group, along with an unselectable group. The sp-menu-group elements with the selects attribute present should fire a change event when their selection state changes, while the action menu would not as the handling is managed by the menu group.

<sp-action-menu>
	<sp-menu-group selects="multiple">
		<sp-menu-item selected>multiitem 1</sp-menu-item>
		<sp-menu-item>multiitem 2</sp-menu-item>
		<sp-menu-item selected>multiitem 3</sp-menu-item>
	</sp-menu-group>
	<sp-menu-group selects="single">
		<sp-menu-item selected>singleitem 1</sp-menu-item>
		<sp-menu-item>singleitem 2</sp-menu-item>
		<sp-menu-item>singleitem 3</sp-menu-item>
	</sp-menu-group>
	<sp-menu-group>
		<sp-menu-item>action 1</sp-menu-item>
		<sp-menu-item>action 2</sp-menu-item>
		<sp-menu-item>action 3</sp-menu-item>
	</sp-menu-group>
</sp-action-menu>

Example 2 (contrived case)

While this example is contrived, it helps illustrate the ownership & management. Since the overall action menu has selects="single", the unmanaged menu group for items 4 & 5 should be affected by that single select. Items 1-3 are managed by the multi-select menu group though. Clients should manage cases like this at the menu group level though.

<sp-action-menu selects="single">
	<sp-menu-group selects="multiple">
		<sp-menu-item>item 1</sp-menu-item>
		<sp-menu-item>item 2</sp-menu-item>
		<sp-menu-item>item 3</sp-menu-item>
	</sp-menu-group>
	<sp-menu-group>
		<sp-menu-item>item 4</sp-menu-item>
		<sp-menu-item>item 5</sp-menu-item>
	</sp-menu-group>
 </sp-action-menu>

### Core sp-menu change

To support the above examples, the `sp-menu` element will also add a `selects` attribute. Since these are now created/managed by `sp-action-menu`, `sp-picker`, & `sp-split-button` internally, they can just set the attribute properly for the component's context.
@adixon-adobe
Copy link
Collaborator Author

@Westbrook regarding our discussion about the selectable attribute on sp-menu, I think at the very least we'd need an sp-menu-group override tied to the selects attribute. I think with the re-parenting of sp-menu though, keeping the selectable attribute you have now makes sense (and just setting it to the right thing in the parent attribute). I don't love that sort of management in general, but it definitely makes sense here.

@Westbrook
Copy link
Contributor

Your approach to selectable makes sense here. The one alternative to contemplate there is whether we move from [selectable] to [selects] in the selector for this, which could be possible depending on your response to the following question on whether there's a typo in your examples or we had a difference in understanding our conversation earlier...

This version implies that sp-action-menu manages the selection:

<sp-action-menu selects="single|multiple">
	<sp-menu>
                ...
        </sp-menu>
</sp-action-menu>

In this world would we also be able to leverage selects on sp-menu itself in other use contexts:

<sp-menu selects="single|multiple">
        ...
</sp-menu>

In that case would three elements have the selects API: sp-action-menu, sp-menu, sp-menu-group?

Is there a specific use case that would become unavailable if only sp-menu and sp-menu-group surface this API?

@adixon-adobe
Copy link
Collaborator Author

Ahhh -- I hadn't quite rationalized that. I do find it awkward that this is possible to write though:

<sp-picker>
  <sp-menu selects="multiple">
      ....
   </sp-menu>
</sp-picker>

How crazy is it to have the parent control create the sp-menu in the shadow dom? I feel like there was some deeper discussion about that a while back.

@Westbrook
Copy link
Contributor

For the parent to create the sp-menu we’d need to upgrade the reparenting to work against more than a single element at a time, or have appropriate recursion. I am not against it in anyway, but I’m not able to speak to the actual complexity therein. I am very interested in seeing the reparenting functionality abstracted and massaged for use through multiple slots if we go this direction. That’s not to say I’d need you to do that, but that I’d want you to clarify the path to making that possible in future work.

@adixon-adobe
Copy link
Collaborator Author

I'll carve out some time on Friday to do a spike here. While the sp-menu / re-parenting change doesn't seem necessary to address this issue, I'd also rather avoid too much unnecessary API churn if we can help it, and at least from a "clean API" standpoint (ignoring complexity), removing the need for sp-menu in these contexts seems like a better path.

Which is to say "I'll probably talk myself out of this".

adixon-adobe added a commit that referenced this issue Mar 3, 2021
I'd do more on the manageSelection changes here, but I think we can
remove a lot of this by using new options from #1189
adixon-adobe added a commit that referenced this issue Mar 4, 2021
I'd do more on the manageSelection changes here, but I think we can
remove a lot of this by using new options from #1189
adixon-adobe added a commit that referenced this issue Mar 4, 2021
I'd do more on the manageSelection changes here, but I think we can
remove a lot of this by using new options from #1189
adixon-adobe added a commit that referenced this issue Mar 9, 2021
I'd do more on the manageSelection changes here, but I think we can
remove a lot of this by using new options from #1189
Westbrook pushed a commit that referenced this issue Mar 9, 2021
I'd do more on the manageSelection changes here, but I think we can
remove a lot of this by using new options from #1189
@adixon-adobe
Copy link
Collaborator Author

I updated the examples above to remove sp-menu from the controls it's deprecated in, and added another section to explain that sp-menu will actually power this behavior.

@Westbrook
Copy link
Contributor

We're getting close to a stable version of this, the main difference I'm running into is I don't think we can "inherit" by default and we need to make that explicit. See: https://westbrook-menu-selects--spectrum-web-components.netlify.app/storybook/?path=/story/menu-group--inherit

In this way the contrived example above would require the following:

<sp-action-menu selects="single">
	<sp-menu-group selects="multiple">
		<sp-menu-item>item 1</sp-menu-item>
		<sp-menu-item>item 2</sp-menu-item>
		<sp-menu-item>item 3</sp-menu-item>
	</sp-menu-group>
	<sp-menu-group selects="inherit">
		<sp-menu-item>item 4</sp-menu-item>
		<sp-menu-item>item 5</sp-menu-item>
	</sp-menu-group>
 </sp-action-menu>

This is to easily clarify the state for the element to understand what it is doing and to ensure that the styling required for Menu Items that can be selected is applied without needing to autonomously sprout attributes. This also ensure that the API for groups is roughly the same as the API for root menus which should make them easier to understand to a developer leveraging the API.

In the case of the original contrived code, without the <sp-menu-group selects="inherit"> the Menu Group would be listed as "making no selections" and it would take ownership of it's child items, which would prevent the root Menu from being able to do so. I've gone back and forth on the ease of use here. Certainly, inherit feels like the easier default, but in that it requires the attribute for styling it makes it really difficult for a consuming developer to make the Menu Group no select, so I keep coming back to this as the correct default.

@adixon-adobe
Copy link
Collaborator Author

I had a lot of trouble figuring out the right default here as well, and you know more about the different concerns here. This sounds good to me!

@Westbrook Westbrook unpinned this issue Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants