feat(ui5-menu): menu item groups with checkable menu items #10028
Conversation
nnaydenow
dismissed
their stale review
Menu tests are fixed and this PR no longer need to be blocked
|
|
||
| const newState = !this.checked; | ||
|
|
||
| this.fireDecoratorEvent("item-check"); |
There was a problem hiding this comment.
The event should be fired after the state has been updated. When the event is triggered, its target must reflect the component’s current state (i.e., checked).
Note: If this event becomes cancelable in the future, a revert mechanism will need to be implemented.
There was a problem hiding this comment.
This is already obsolete as the event is made private now.
There was a problem hiding this comment.
I would agree with Nayden that the only valid reason for firing the event before changing the value is if you want the event to be preventable. And furthermore if you fire the event before changing the value you should provide the new value.
NHristov-sap
changed the title
| @@ -0,0 +1,134 @@ | |||
| import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js"; | |||
There was a problem hiding this comment.
General remark:
As there is no public event as far I can see from the change, the apps won't be able to handle changes in the checked state of menu items inside menu groups and update their models on interaction. Or, are there other means addressing this?
I think we need to provide a public event in the MenuItemGroup - something like selection-change. But as we use checked in this case, so it should be item-check or check-change. The second is more consistent, but sounds odd - it's for the KM's to review it as well.
No matter the name, it will be good if the event provide some details to get the checked items from the event - e.detail.checkedItems, f.e:
type MenuItemGroupItemCheckEventDetail = {
checkedItems: Array<IMenuItem>;
}*If you like item-check, you need to rename the private event of the MenuItem from item-check to check or _item-check or something else.
There was a problem hiding this comment.
My opinion is that additional event is not needed - now we have item-click event. The check/uncheck happens on click of the item, so this event can notify for check state changes. The clicked item is provided in the event detail. The developer can check item's checked property and can update the model.
There was a problem hiding this comment.
Unresolved by your previous comment above where you said that the item-check event is now private.
| } | ||
|
|
||
| return super.focus(focusOptions); | ||
| } |
There was a problem hiding this comment.
The method was moved without any modifications. In the context of such a large change, this creates unnecessary clutter in the diff.
| return true; | ||
| } | ||
|
|
||
| get isRtl() { |
There was a problem hiding this comment.
the same as above. No need for moving the method
| return items; | ||
| } | ||
|
|
||
| onBeforeRendering() { |
There was a problem hiding this comment.
This method was moved without any changes. As a general recommendation, I suggest avoiding moving methods at all—especially in large changes—since it adds unnecessary noise to the diff and makes the review harder to follow. I won't comment on each moved method individually, but this feedback applies to all such cases.
You can always re-order the methods with later change. Although I also do not recommend this because it makes following the history harder.
| * the last checked item will take precedence. | ||
| * @private | ||
| */ | ||
| _ensureSingleItemIsChecked() { |
There was a problem hiding this comment.
[MAJOR] To be checked with core team: The overall design of this method is flawed. It modifies the checked property of all items directly without notifying the application, which can cause the UI to fall out of sync—especially in a React environment where state synchronization is critical.
|
|
||
| const newState = !this.checked; | ||
|
|
||
| this.fireDecoratorEvent("item-check"); |
There was a problem hiding this comment.
I would agree with Nayden that the only valid reason for firing the event before changing the value is if you want the event to be preventable. And furthermore if you fire the event before changing the value you should provide the new value.
| @@ -0,0 +1,134 @@ | |||
| import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js"; | |||
There was a problem hiding this comment.
Unresolved by your previous comment above where you said that the item-check event is now private.
| const siblingsWithIcon = this._menuItems.some(menuItem => !!menuItem.icon); | ||
| const siblingsWithIcon = this._allMenuItems.some(menuItem => !!menuItem.icon); | ||
|
|
||
| this._setupItemNavigation(); |
There was a problem hiding this comment.
Is this really needed for checked state feature? If not better separate in a new change. And all related code to item navigation
This PR introduces MenuItemGroup component that can hold regular MenuItem components. The MenuItemGroup has a property named
itemCheckModewhich can have values amongNone(default),SingleandMultiple. MenuItemGroup can be slotted in a Menu or MenuItem default slot as any other regular MenuItem. Nesting of MenuItemGroup components is not allowed, but any Menu or MenuItem can contain more than one MenuItemGroup components with differentitemCheckModesettings.When
itemCheckModeis:None, the Menu acts exactly like until now.Singlemeans that zero or one MenuItems can be checked at a time.Multiplemeans that zero or many MenuItems can be checked at a time.There is also new property
checkedintroduced in MenuItem. By setting it the item is marked as checked 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 withSingleorMultiplevalue ofitemCheckMode.It is recommended to place separators before and after each MenuItemGroup, but this is not mandatory and depends on the application developers.