Why don't Items have an onAction event?

[GermanJablo]

Context: the menu component has an event called onAction that receives the key of the pressed item as a parameter (source).

This seems unnecessarily complicated because a relationship must be maintained between the event and the keys.

I see there is work being done on allowing href. But better yet,

Why not provide an onPress event on each item instead? EDIT: onAction, not onPress.

[snowystinger]

There are multiple ways to potentially trigger or not trigger an onAction across different kinds of collections. For instance, Menus will eventually have submenus. Pressing on the item which opens a submenu should not fire an onAction, but onPress doesn't have that intent captured in it. So would a submenu trigger still call onPress? Or is it meant to represent an action?

In addition, collections also respond to different events following aria patterns and, absent that, native examples. So menu items for keyboard actually call onAction during onPressStart, while touch and mouse call it during onPressUp. If we just provided press props, it'd be up to the implementer to guess the correct one to use in each situation, and guessing wrong could be bad, as we would still be handling closing the menu.

[snowystinger]

The aria pattern calls for both actions/activating as well as selection, of which there can be multiple models. See https://www.w3.org/WAI/ARIA/apg/patterns/menubar/#keyboardinteraction
In some cases the menu should stay open, and in others it should close. We wouldn't know to do that if the selection state was removed from Menu and it'd be another thing that people would have to control.

Other components make action vs selection more obvious. See https://react-spectrum.adobe.com/react-aria/Table.html#row-actions

Your example only looks at the static case. There is also the dynamic case to consider. See https://react-spectrum.adobe.com/react-aria/Menu.html#content

As for other collections, some routinely have thousands of items in them. It's unreasonable to expect someone to pass a handler to each one. For one, it makes it harder to keep an updated reference to the handler in each Item, especially if we are caching, which we do automatically to help with cases of thousands of items. And two, it's likely they'd just pass the handler which aggregates everything, so right back to tracking those keys.

I don't disagree that for the small menu static case, this could be nice. However, our team decided that a single expected place for all of these props and events made the most sense given everything we've talked about.

[GermanJablo]

Thank you for clarifying the selection issue for me.

It occurs to me that Items could have an initialSelection or initialSelected property (defaults to false):

<Item initialSelection={true} onAction={handleLeft}> Left </Item>

For the dynamic case, both the onAction and the initialSelection could be defined like this:

let items = [
  {action: handleNew, name: 'New'},
  {action: handleOpen, name: 'Open'},
  {action: handleClose, name: 'Close', initialSelected: true},
  {action: handleSave, name: 'Save'},
  {action: handleDuplicate, name: 'Duplicate', initialSelected: true},
  {action: handleRename, name: 'Rename'},
  {action: handleMove, name: 'Move'}
];

In fact, something like this is what I have managed to do in my project with a custom wrapper. But in addition to the fact that it took work, I can't use it in a static way.

Regarding collections with thousands of items whose actions are virtually the same, I'm not sure it's a worse DX. Yes, you are defining a handler, but you are actually replacing it with the definition of an id.

// Instead of this:
let items = [
     {id: 1, name: 'New'},
     {id: 2, name: 'Open'},
     {id: 3, name: 'Close'},
   ];

// This doesn't seem so bad:
let items = [
     {action: alert(1), name: 'New'},
     {action: alert(2), name: 'Open'},
     {action: alert(3), name: 'Close'},
   ];

On the other hand, if the handlers are few or the actions are different, the second option is clearly better, since the pairing does not have to be maintained. I mean, it's kind of like why Tailwind is so popular. You don't have to go around matching DOM elements with CSS through classes or ids. You are collocating both things in the same place. The same here.

Finally, if someone has a very specific use case where they need to hook onSelectionChange, they could add an id to each Item and use onSelectionChange on the menu as is done now. But I'm sure that will be -1% of the cases.

[snowystinger]

We have a few reasons for not having things like isSelected at the Item level:

• Much like onAction, Items can work in components that may not support selection
• Keeping it at the root level allows users to mark keys as selected before they are even loaded from the server (e.g. in an infinite scroller). That usecase is listed on our Collections docs page.

Also, for something like onAction, sometimes that needs to be triggered for an item from the root component, and not as a result of a DOM event on the item itself. For example in Combobox, the Enter key is not fired on the Item because focus does not go there. This also happens with selection when using the keyboard to select multiple.

[GermanJablo]

Coming back to this after a while and having seen that different Items are now used for different collections: #5317

If I'm not mistaken, that solves your first and last objection. I'm not too sure about the "selected keys before the items are loaded from the server" thing though.

[emlai]

I'm having a related problem: #5403. onAction / onClick on menu items would easily solve my use case.

[GermanJablo]

hey @snowystinger I'm writing in a new comment because in the comment thread above I mixed up some things and there were others that I didn't understand well. I've been thinking about this and I think I've come up with a better way to solve these problems:

Problems

1. Items require an id, and then the user usually has to access the item again from the id, resulting in a convoluted and repetitive API.
2. The current API forces items to store their states in a different space or form. For example, if my items objects have properties like selected or collapsed, I still need to replicate these states in a separate object with selectedKeys or collapsedKeys.

Solution:

For each state of an item (selected, collapsed, etc.), a setter and a getter that receive the item should be exposed.

Taking the case of selection as an example, if you think about it actually onSelectionChange fulfills the role of setter (although it receives a key instead of the item object). selectedKeys and defaultSelectedKeys could be replaced by a getter.

// replace this (current API)
type CollectionProps = {
  defaultSelectedKeys: 'all' | Iterable<Key>,    // initial state
  selectedKeys: 'all' | Iterable<Key>,           // current state
  onSelectionChange: (keys: Selection) => any,   // setter
  onAction: (key: Key) => void,
}

// with this (new API)
type CollectionProps = {
  getSelected: (item: object) => boolean,        // getter
  setSelected: (item: object) => void,           // setter
  getAction: (item: object) => () => void,       // getter
}

The solution that I proposed before (in the thread above) was to move the handling of actions or selection, from the collections to the items.
This solution is better and has several benefits such as:

1. The collection is still in charge of state management, and not of the items
2. Does not require items to have an id.
3. It does not impose a specific interface or model for the items on users. That is, the selection state, expanded or anything else can live in the item object itself, or outside as each person prefers.
4. No defaultSelectedKeys or defaultWhateverKeys type properties are required.

[snowystinger]

Thanks for the added thoughts. The team restarted discussions on this topic after we made the change to Item. I'll hopefully have an update here eventually, though we do have some large priorities that we're working on right now.

In general, and just as a heads up, we're unlikely to make any removals to the API right now, though that could change down the road.