notes icon indicating copy to clipboard operation
notes copied to clipboard

Add FullWidthMenu

Open ghost opened this issue 5 years ago • 5 comments

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 avatar Jun 15 '20 08:06 ghost

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

ismay avatar Jun 15 '20 08:06 ismay

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

HendrikThePendric avatar Jun 15 '20 10:06 HendrikThePendric

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.

ismay avatar Jun 18 '20 07:06 ismay

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.

ismay avatar Jun 18 '20 07:06 ismay

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.

HendrikThePendric avatar Jun 18 '20 08:06 HendrikThePendric