Intro
The current version of the Menu and related components have a few issues that could be improved. Some changes were made in the past with a big focus on maintaining backwards compatibility (dhis2/ui-core#297 and dhis2/ui-core#354). We discussed on Slack that some more fundamental improvements couldn't be done without introducing breaking changes, and since we are now building up to a next major version we can now break 👹 and improve stuff. I'll go through the aspects of these components where there's room for improvement and conclude with a list of components I think we should implement for v5.
I had previously began to discuss various options for this with @Mohammer5 on Slack.
Making the menu click based
The current implementation responds to hover, and we discussed this needs to be changed to a click based implementation. For a click based menu we'd expect the following functionality:
- Given that a menu has multiple sub-menus (at one level) and one is open, we'd expect that one to close when another sub-menu is opened (toggling). To achieve this we need to control which sub-menu is open from the parent menu.
- When the user clicks outside of the menu, we'd expect all of the open sub-menus to close. Looking at the design specs, this behaviour doesn't have to be provided by the menu itself. Instead, the app would render a menu in a Popover and when the Popover's backdrop-layer is clicked would hide the Popover and the Menu as well.
Distinguishing between a menu-item and a sub-menu
Currently we only have a MenuItem component which has various props that determine the content of the menu-item, such as label and icon. And if the MenuItem has children then we assume it's a sub-menu. A dedicated SubMenu component would make things a lot more explicit, provide a clear separation of concerns, and would allow for the MenuItem to just render its children, which would be more in line with the rest of our components.
Separating behaviour from style
A fly-out menu has pretty opinionated behaviour. At the same time, a menu-list with menu-items could be a fairly generic way to present things. I've discussed this with @cooper-joe and he informed me that he presently knew of no other use-cases than outlined in the design-specs: a dropdown-style fly-out menu. But we both could see a plain menu-list with menu-items being used in a sidebar. So it would make sense to have a clear split between presentational and functional components, and this also fits very well with the distinctions between the packages in this mono-repo.
Dedicated components to reflect the design system
The design system specifically mentions dividers and section headers. We already have a generic Divider component which has the correct styling. For correct semantics we'd have to make sure that this is also wrapped in a li tag (given that the parent is an ul). And we'll definitely need to introduce a dedicated MenuSectionHeader component.
Allowing flexible menu-item content
One thing is for certain: the root element for a menu-item should be a li, because it is a direct descendant of an ul. However, wrapping the content in a a is perhaps not always what you would want to do, for example when using react-router, or when you are refactoring an app that is wrapping stuff in a tags already (?). One way to provide convenience and flexibility would be to use the pattern we established building the Tooltip. This would mean that the menu-item would render its children into an anchor by default, and if you want to opt-out of that, you can use the renderProps approach.
Suggested component set
Based on the summary above I've come up with a set of components. Quite likely, the details may change when I start implementing things, and when I receive input on this issue:
Presentational (i.e. ui-core or whatever name comes out on top):
MenuList: an ul with appropriate styles that just renders childrenMenuListItem: a li with appropriate styles depending on propsMenuSectionHeader: a li with perhaps a h6 and some specific stylingMenuDivider: a li with a Divider
Behavioural:
Menu: renders children into a Card with a MenuList and controls the toggling of the SubMenu open stateMenuItem: by default this will render its children into a MenuListItem, but also supports renderProps for custom content.SubMenu: renders a MenuItem (I think via a menuItem prop. And conditionally renders a fly-out menu, which will be toggled on click.
Intro
The current version of the Menu and related components have a few issues that could be improved. Some changes were made in the past with a big focus on maintaining backwards compatibility (dhis2/ui-core#297 and dhis2/ui-core#354). We discussed on Slack that some more fundamental improvements couldn't be done without introducing breaking changes, and since we are now building up to a next major version we can now break 👹 and improve stuff. I'll go through the aspects of these components where there's room for improvement and conclude with a list of components I think we should implement for v5.
I had previously began to discuss various options for this with @Mohammer5 on Slack.
Making the menu click based
The current implementation responds to hover, and we discussed this needs to be changed to a click based implementation. For a click based menu we'd expect the following functionality:
Distinguishing between a menu-item and a sub-menu
Currently we only have a
MenuItemcomponent which has various props that determine the content of the menu-item, such aslabelandicon. And if theMenuItemhaschildrenthen we assume it's a sub-menu. A dedicatedSubMenucomponent would make things a lot more explicit, provide a clear separation of concerns, and would allow for the MenuItem to just render its children, which would be more in line with the rest of our components.Separating behaviour from style
A fly-out menu has pretty opinionated behaviour. At the same time, a menu-list with menu-items could be a fairly generic way to present things. I've discussed this with @cooper-joe and he informed me that he presently knew of no other use-cases than outlined in the design-specs: a dropdown-style fly-out menu. But we both could see a plain menu-list with menu-items being used in a sidebar. So it would make sense to have a clear split between presentational and functional components, and this also fits very well with the distinctions between the packages in this mono-repo.
Dedicated components to reflect the design system
The design system specifically mentions dividers and section headers. We already have a generic
Dividercomponent which has the correct styling. For correct semantics we'd have to make sure that this is also wrapped in alitag (given that the parent is anul). And we'll definitely need to introduce a dedicatedMenuSectionHeadercomponent.Allowing flexible menu-item content
One thing is for certain: the root element for a menu-item should be a
li, because it is a direct descendant of anul. However, wrapping the content in aais perhaps not always what you would want to do, for example when using react-router, or when you are refactoring an app that is wrapping stuff inatags already (?). One way to provide convenience and flexibility would be to use the pattern we established building the Tooltip. This would mean that the menu-item would render its children into an anchor by default, and if you want to opt-out of that, you can use the renderProps approach.Suggested component set
Based on the summary above I've come up with a set of components. Quite likely, the details may change when I start implementing things, and when I receive input on this issue:
Presentational (i.e. ui-core or whatever name comes out on top):
MenuList: anulwith appropriate styles that just renders childrenMenuListItem: aliwith appropriate styles depending on propsMenuSectionHeader: aliwith perhaps ah6and some specific stylingMenuDivider: aliwith aDividerBehavioural:
Menu: renders children into aCardwith aMenuListand controls the toggling of theSubMenuopen stateMenuItem: by default this will render its children into aMenuListItem, but also supports renderProps for custom content.SubMenu: renders aMenuItem(I think via amenuItemprop. And conditionally renders a fly-out menu, which will be toggled on click.