-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add FullWidthMenu #178
Comments
If we implement this I could add a special note to the release post as well. |
Alternative suggestion:
|
That 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.
|
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.
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).The text was updated successfully, but these errors were encountered: