Open ismay opened 4 years ago
If we implement this I could add a special note to the release post as well.
Alternative suggestion:
Menu
: is full-width and encapsulates all the Menu behaviour, incl. fly-out sub-menusMenuCard
: renders a Menu in a Card and takes care of the width/height restrictionsThat could also make sense. If we go with Menu and MenuCard, my assumption as a user would be the following:
With the names I suggested my assumption as a user would be that they're on the same level of abstraction, where the names indicate exactly what the difference is between them:
I think what we're ultimately dealing with here is establishing a clear scope for these components. I think we're having problems with the naming because apparently parts of the scope are still unclear. It seems we have the following features:
In thinking about it (and without knowing the Menu internals, so this is purely from the perspective of a user), I think that these could also make sense implemented in a single generic Menu component, instead of different Menu subtypes.
wrapped
, or card
, something of that nature).Note that the above proposal is purely me trying to sketch out what seems clear from my perspective, so we can start establishing what would be the ideal from our perspective. I can imagine that for practicality's sake we'd make compromises, to not break the Menu this quickly after a breaking change.
Yeah, I think we are both suggesting the same thing, and are now discussing the details. I agree the Menu should be fullwidth and encapsulate all the behaviour.
We could outsource wrapping in a card to the user. Or expose a prop for convenience that does this (wrapped, or card, something of that nature).
I think it'd be nice to keep a dedicated component for this, since some of the size restrictions come from the design system.
One other related problem that I think is present in the current solution already, is if you would want to control the dimensions of the sub-menus. Say you have some deeply nested sub-menu that has very long labels. You might want to increase the width on that one. Currently this is not possible. To solve this we could expose some props on the MenuItem. (actually, in my mind this would be an argument for having a dedicated SubMenu component as I had initially, but let's forget about that). This is something we can introduce as a non-breaking change.
See here: https://dhis2.slack.com/archives/CBM8LNEQM/p1591949691034800. We have had the following changes in the Menu for v5:
MenuList
->Menu
Menu
->FlyoutMenu
This can create confusion when a user migrates from 4 to 5, and misses the breaking changes in the Menu. They'll end up with a fullwidth menu when in fact they wanted the flyoutmenu.
To remedy this I think it makes sense to move to:
FullwidthMenu
FlyoutMenu
This clearly states the responsibilities of each type of menu. The
Menu
name now could lead one to believe that it is somehow more generic than theFlyoutMenu
, is slightly confusing if you're coming from v4 and could also lead to scope creep due to the more generic name.I propose we add the
FullwidthMenu
in a feature bump, and deprecate theMenu
(and remove it in v6). We could add a console warning whenever theMenu
is used, so people will be warned when they don't migrate it to theFullwidthMenu
(but it won't break).