Skip to content
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

Open
ghost opened this issue Jun 15, 2020 · 5 comments
Open

Add FullWidthMenu #178

ghost opened this issue Jun 15, 2020 · 5 comments

Comments

@ghost
Copy link

ghost commented Jun 15, 2020

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 the FlyoutMenu, 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 the Menu (and remove it in v6). We could add a console warning whenever the Menu is used, so people will be warned when they don't migrate it to the FullwidthMenu (but it won't break).

@ghost
Copy link
Author

ghost commented Jun 15, 2020

If we implement this I could add a special note to the release post as well.

@HendrikThePendric
Copy link
Contributor

Alternative suggestion:

  • Menu: is full-width and encapsulates all the Menu behaviour, incl. fly-out sub-menus
  • MenuCard: renders a Menu in a Card and takes care of the width/height restrictions

@ghost
Copy link
Author

ghost commented Jun 18, 2020

That could also make sense. If we go with Menu and MenuCard, my assumption as a user would be the following:

  • Menu: this is the base Menu, a generic Menu, that has all the features
  • MenuCard: this is a variant of the base menu, a more specific version with less features

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:

  • FullwidthMenu: this seems straightforward. Does not tell me if it can have submenus though.
  • FlyoutMenu: you need to know what a flyout menu is. It implies that this can have submenus. Does not tell me that it will be displayed in a card.

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:

  • Submenus: yes/no
  • Wrapped in card: yes/no
  • Width: constrained or full width

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.

  • We could always enable submenus (because if a dev uses them, apparently they want to use them).
  • 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).
  • Root menu Width could be full width by default, like we do with our other components. I think Viktor established that they should always be full width, and that width should/can be constrained by the dev with the Box for our core components. There might be a nuance that I'm missing, but I believe that that was the gist. Submenus would of course still be constrained, since they hover.

@ghost
Copy link
Author

ghost commented Jun 18, 2020

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.

@HendrikThePendric
Copy link
Contributor

HendrikThePendric commented Jun 18, 2020

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.

@varl varl transferred this issue from dhis2/ui Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants