feat(ui5-menu): refactor menu items display #8259
Conversation
- This PR affects the ui5-menu and ui5-side-navigation components rendering. - The ui5-menu-item now extends ui5-li-custom class and it will be represented directly as a list item in the DOM. As a result the ui5-menu-item is no longer an abstract class, but a physical component and it will ease the possibility in the future to support custom content supplied by the application developers. - Additionally the ui5-reponsive-popover is no longer rendered into the static area. Thus the ui5-menu structure defined by the application developers is represented more accurately into the DOM and the browser popover API could be used once available for all supported browsers. - There is also a new behavior implemented, which enables the end users to navigate via mouse and keyboard to a disabled ui5-menu-item element. This behavior is aimed to improve the accessibility of the ui5-menu component by providing screen reader announcements for disabled menu items as well. Fixes: SAP#7096
| </div> | ||
| </ui5-li> | ||
| {{/each}} | ||
| <slot></slot> |
There was a problem hiding this comment.
Do we need them as a slot or we need them as individual ones?
There was a problem hiding this comment.
Actually it seems to me now that I've read the documentation that the individual slot scheme would be more suitable for the ui5-navigation-menu template as there is an anchor tag wrapper around most of the ui5-navigation-menu-item's:
Sometimes, however, each child must be placed separately in the shadow root, potentially wrapped in other HTML elements, to satisfy the UX design of the component.
The currently implemented slot scheme in the ui5-menu looks clean. Not sure yet if we could use the general slot approach in the ui5-menu and at the same time individual slot approach for the ui5-navigation-menu derived component.
Additional feedback would be greatly appreciated.
| const item = e.target as MenuItem; | ||
| item.hasSubmenu && await this._prepareSubMenuDesktopTablet(item); | ||
| } else if (shouldCloseMenu && this._isSubMenu && this._parentMenuItem) { | ||
| const parentItemMenu = this._parentMenuItem.parentElement as Menu; |
There was a problem hiding this comment.
Can we have that interaction trough events? For example the menu to throw some event to the parent menu like "close-submenu" and then the parent to close it. I don't think that reaching to the parent and calling a method of the parent is good practice.
There was a problem hiding this comment.
Yes, that is a good idea now that the ui5-menu-item's do have a DOM representation and they aren't displayed in the static area. My only concern is that it could take additional time to implement and if we'll fit in the time frame. I'll try to evaluate this effort and we could have a follow up discussion about it and make a decision if we'll implement it now or in an additional task.
|
Hi @unazko, @tsanislavgatev, Please play a bit with following scenario with the proposed implementation: |
The
ui5-menu-itemnow extendsui5-li-customclass and itwill be represented directly as a list item in the DOM.
As a result the
ui5-menu-itemis no longer an abstract class,but a physical component and it will ease the possibility
in the future to support custom content supplied by
the application developers.
The
ui5-reponsive-popoveris no longerrendered into the static area. Thus the
ui5-menustructuredefined by the application developers is represented more
accurately into the DOM and the browser
popoverAPI couldbe used once available for all supported browsers.
There is now no differentiation between mobile and desktop
device in regards to the display mechanism. In both cases
we rely on the template to do the job as the components used
for composition like
ui5-listandui5-responsive-popoverdocomply with the device.
There is also a new behavior implemented, which enables
the end users to navigate via mouse and keyboard to a
disabled
ui5-menu-itemelement. This behavior is aimedto improve the accessibility of the
ui5-menucomponentby providing screen reader announcements for disabled
menu items as well.
The
haspopupattribute is now applied to theui5-menu item'swith a sub-menu.
The
ui5-menuelements used for sub-menus are created only onceand are being reused afterwards. They are no longer destroyed on close.
This contributes to lowering the count of the slow DOM manipulation operations.
The custom focus handling is now done solely in the
ui5-menucomponentas we no longer use the
ui5-responsive-popoverfunctionalities to retrieve the focus.As a result the code is easier to maintain and trace the focus shifting.
Fixes: #7096
Fixes: #7767
Fixes: #7423
Related to: #6350
Related to: #8214
Related to: #7391