feat(ui5-menu): selectable menu items #10028
Conversation
nnaydenow
dismissed
their stale review
Menu tests are fixed and this PR no longer need to be blocked
|
|
||
| this.items.forEach(item => { | ||
| if (item.isGroup) { | ||
| items.push(...(item as MenuItemGroup)._menuItems); |
There was a problem hiding this comment.
For me filtration of menu items should happen here since it doesn't know this context and simply items property to be used.
There was a problem hiding this comment.
Group also can contain separators, which are not regular itrems, that's why I want to be safe and use MenuItemGroup's _menuItems getter which returns only regular items
packages/main/src/MenuItem.ts
Outdated
| * @private | ||
| */ | ||
| @property() | ||
| _itemSelectionMode: `${ItemSelectionMode}` = ItemSelectionMode.None; |
There was a problem hiding this comment.
selection mode for consistency with all other components that provide selection
There was a problem hiding this comment.
MenuItem extends ListItem (where selectionMode already exists, but with different behavior, so mixing is not possible here.
packages/main/src/MenuItem.ts
Outdated
| * **Note:** A selected `ui5-menu-item` have selection mark displayed ad its end. | ||
| * @default false | ||
| * @public | ||
| * @since 2.4.0 |
There was a problem hiding this comment.
since 2.5.0
why do we need setter and getter? they do synchronous property change and it should be used in very edge cases.
overall why do we need isSelected property which leads to inconsistency with other components that provide selection? what is the difference between selected and isSelected overall?
There was a problem hiding this comment.
Setter is needed because if you select item that is inside a MenuItemGroup with Single selection mode, other selected items should be deselected automatically by the group. Here we fire item-selection event that is handled by group and if it is necessary (selection mode is Single) the group unselects other items.
As I already wrote as answer of Ilhan's comment, the MenuItem extends ListItem that also have selection property, but it is related to totally different behavior, so selection here is different and we need different property for this. selected in ListItem does this:
and isSelected in MenuItem t=does this:
and at the same time we can have "original" ListItem selected state in Menu like here:
packages/main/src/MenuItemGroup.ts
Outdated
| * @public | ||
| */ | ||
| @property() | ||
| itemSelectionMode: `${ItemSelectionMode}` = ItemSelectionMode.None; |
There was a problem hiding this comment.
same as in the MenuItem.
Add also since tag and use plain string as initializer.
There was a problem hiding this comment.
Same here, selectionMode belongs to ListItem with different meaning and behavior
Regarding the @since - the whole component is new, and already have @since tag.
| _itemClick(e: CustomEvent<ListItemClickEventDetail>) { | ||
| const item = e.detail.item as MenuItem; | ||
| const prevSelected = item.isSelected; | ||
|
|
||
| item.isSelected = !prevSelected; |
There was a problem hiding this comment.
I think the MenuItem needs a new property "selected", you will also benefit from it in the styles as you will be able to use [selected] selector and not adding some additional classes. And you won't need isSelected as well.
There was a problem hiding this comment.
MenuItem extends ListItem which already have selected property, but selected in ListItem has totally different behavior, so I can neither use selected nor reuse related CSS. So I definitely need another "selected" state property here.
| * @since 2.5.0 | ||
| */ | ||
| @property({ type: Boolean }) | ||
| set isSelected(value: boolean) { |
There was a problem hiding this comment.
isSelected is not ok for a property name. It must be selected. Let's discuss refactoring possibilities to make selected not clash with the list item one.
| */ | ||
| @property({ type: Boolean }) | ||
| set isSelected(value: boolean) { | ||
| this.fireDecoratorEvent("item-selection"); |
There was a problem hiding this comment.
You must only fire events upon user interaction, not when the application sets a property.
Also, custom setters are an anti-pattern, and are only accepted in extreme cases where synchronous behavior is needed (f.e. the Popover opener/open property that must open the popover in the same tick for the iOS soft keyboard to work).
Remove the custom setter, make it a normal property, and fire the event only when the user manually selects/deselects a menu item (and not when the application sets it).
| const items: MenuItem[] = []; | ||
|
|
||
| this.items.forEach(item => { | ||
| if (item.isGroup) { |
There was a problem hiding this comment.
item.isGroup is not a proper typescript check, therefore you need as MenuItemGroup on the next line. Same for other parts of the code. The correct way to do it is for example as done in Side Navigation:
get isSideNavigationItemBase() {
return true;
}
...............
const isInstanceOfSideNavigationItemBase = (object: any): object is SideNavigationItemBase => {
return "isSideNavigationItemBase" in object;
};
You must make a similar instanceof function for MenuItem/Group etc... and then when you do the same check as on line 363, Typescript will know that if the check passed, the type of your item is whatever you put in is, as shown above.
| } | ||
|
|
||
| get _menuItems() { | ||
| return this.items.filter((item) : item is MenuItem => !item.isSeparator); |
There was a problem hiding this comment.
Here it's good, you used is so typescript knows what _menuItems is.
This PR introduces MenuItemGroup component that can hold regular MenuItem components. The MenuItemGroup has a property named
itemSelectionModewhich can have values amongNone(default),SingleSelectandMultiSelect. MenuItemGroup can be slotted in a Menu or MenuItem default slot as any other regular MenuItem. Nesting of MenuItemGroups is not allowed, but any Menu or MenuItem can contain more than one MenuItemGroup components with differentitemSelectionModesettings.When
itemSelectionModeis:None, the Menu acts exactly like until now.SingleSelectmeans that zero or one MenuItems can be selected at a time.MultiSelectmeans that zero or many MenuItems can be selected at a time.There is also new property
isSelectedintroduced in MenuItem. By setting it the item is marked as selected and this is visualized as checkmark at the end of the item. This property is taken into account only when the corresponding item is a member of MenuItemGroup withSingleSelectorMultiSelectvalue ofitemSelectionMode.It is recommended to place separators before and after each MenuItemGroup, but this is not mandatory and depends on the application developers.