mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

[Menu] Support cascading / nested menu #11723

Open janhoeck opened 5 years ago

janhoeck commented 5 years ago

Hello,

I wrote some two advanced components, who can handle Submenuitems. I defined an anchorElement, which will be passed as property to the Mui Menu Component. Is there a way to define where the Popup Element should be displayed? Because now it will just overlap. Maybe I want to calculate it more left or more right.

Here is an demo: https://codesandbox.io/s/9ywowy18k4

Benchmark

oliviertassinari commented 5 years ago

@janhoeck This is a nice demo! You can use the anchor playground to pick the right values. Here is an example: https://codesandbox.io/s/lx1336xrx7

juin-05-2018 19-43-39

Do you want to add such example in the documentation? 😄

janhoeck commented 5 years ago

Thanks for your answer! Its working too when I align my button to the right. I thought the sub menu item will overlap. But it won't. Here is what I mean: https://codesandbox.io/s/x99r38x6rz

Sure, I can add such an example in the documentation. How I do that? 👍 Maybe we can implement a sub menu component in the core code. I think some people would like this!

oliviertassinari commented 5 years ago

How I do that?

:) https://github.com/mui-org/material-ui/blob/master/CONTRIBUTING.md#how-do-i-add-a-new-demo-in-the-documentation

Maybe we can implement a sub menu component in the core code. I think some people would like this!

Looking at your demo on codesandbox, the submenu implementation seems already straightforward. What abstraction do we miss?

evilass commented 5 years ago

WOOOOW cool!

janhoeck commented 5 years ago

@oliviertassinari Uhm just implement a SubMenu Component in the core code so that the user does not have to write it on his own :). Its just an idea :).

I think I have time to add this to the documentation at the evening :)

janhoeck commented 5 years ago

Uhm @oliviertassinari I don't get material-ui running locally. But I removed mobx from the demo and use the normal setState functions. Soooo maybe you could just copy/paste it in the docs?

Demo: https://codesandbox.io/s/l7wm3rjxzm

menu.md Text:

Sub menus

If you want to have a menu with a sub menu tree (for example you want to add a "others" item as the last item of the parent menu) then you can use the anchorElement function combining with Menu and MenuItem.

The EnhancedMenu is a wrapper for the Menu. The EnhancedMenu will iterate over the menuItems prop and create a normal MenuItem or a new Menu with an anchorElement.

{{"demo": "pages/demos/menus/subMenu/Demo.js"}}

oliviertassinari commented 5 years ago

I don't get material-ui running locally

@janhoeck Are you using yarn? Otherwise, you need to npm install each package. Thanks for simplifying the codesandbox! Also, if you could open a pull-request with any changes, I could update it to match your previous comment: https://github.com/mui-org/material-ui/issues/11723#issuecomment-395300458.

janhoeck commented 5 years ago

@oliviertassinari FYI: Until now I did not find time to document it :/ And tomorrow I am 3 days on a business trip.

pavanshinde47 commented 5 years ago

Guys, Please update this. Thank you!!

JeremyGrieshop commented 5 years ago

@janhoeck Any idea how to enhance/correct the following issues with the SubMenu component you implemented?

  1. After expanding a SubMenu, and clicking away from the expanded SubMenu, it will only close the current SubMenu. This seems right, if "clicking away" means clicking its parent. But, the natural thing to do when clicking completely away is to dismiss everything. Consider a deeply nested submenu, where you intentionally need to "click away" N number of times in order to dismiss every menu window.

  2. Somewhat related to (1), but "clicking away" on another parent menu item that has a SubMenu should dismiss the current SubMenu and go ahead and open up the new SubMenu. I suspect figuring out focus would be difficult. See Office UI Fabric's ContextualMenu. They can even open things onMouseOver.

janhoeck commented 5 years ago

@cobbs-totem You can define a boolean in a ContainerComponent. This boolean will be passed into all submenus. When you open a menu, set the boolean value to true. After that check whenever you click away and set this boolean to false. If the boolean is false, than all menus should react to that and closed.

JeremyGrieshop commented 5 years ago

Thanks for the feedback! If I understand your proposal, I think the setback there is that clicking away only generates an event for the deepest child menu. So, the process would have to go in reverse (child onClose() calls props.onClose() to propagate the change upward to parents). This strategy will work when clicking away from all menus, as they will all get notified eventually, and all close out. However, if you click away from a child by clicking on the parent, the child gets the onClose() and tells all parents to close, including the parent in which you clicked inside. I haven't found a workaround for that.

janhoeck commented 5 years ago

Just check in which container the user clicked. If the container is something else than a menu, then close all menus. And when the user clicked on a menu, than close all child menus of the menu

IssueHuntBot commented 5 years ago

@0maxxam0 funded this issue with $20. See it on IssueHunt

janhoeck commented 5 years ago

@0maxxam0 funded this issue with $20. See it on IssueHunt

The task is already completed. Here I have created a new sandbox for you: https://codesandbox.io/s/509wvp6qxk

KJoyner commented 5 years ago

In the examples above, I am having trouble getting keyboard traversal to work in the nested menu. Any suggestions?

Jak3b0 commented 5 years ago

Hi,

does it support close all the sub-menus and menu when clicking outside of it? Right now, we need to click 2 times to close the whole menu.

Also, is possible to close all the sub-menu/menu when a user click on an item?

Thank you!

janhoeck commented 5 years ago

@Jak3b0 You can do it with the ClickAwayListener. Take a look https://codesandbox.io/s/6voj4o458n

Jak3b0 commented 5 years ago

Thanks Jan!

On Fri, Jan 25, 2019 at 7:12 AM Jan Höck notifications@github.com wrote:

@Jak3b0 https://github.com/Jak3b0 You can do it with the ClickAwayListener. Take a look https://codesandbox.io/s/6voj4o458n

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mui-org/material-ui/issues/11723#issuecomment-457554390, or mute the thread https://github.com/notifications/unsubscribe-auth/ACtpmYf6fW-oSYXokZ59DezpUaKzt3LKks5vGvSzgaJpZM4Ua2JD .

varshasaha commented 5 years ago

Can I take this up?

eps1lon commented 5 years ago

Can I take this up?

That would be really great. The goal is to add a SubMenu (or cascading menu in material speak) to the demos. #8152 and https://github.com/mui-org/material-ui/issues/14345#issuecomment-458888753 are related.

JeremyGrieshop commented 5 years ago

I agree, this would be awesome to have! I created a similar SubMenu component to do this, but it lacks all of the nuances with clicking away. For example, clicking away from all should close, but clicking away on another parent SubMenu should close all up to the clicked menu item.

I also kinda wish the ability to open on hover was an option, but that's probably for another task. Hover open gives you a much more responsive, desktop-like experience. Clicking and waiting for menu popup transitions takes forever when you need to navigate submenus 3 or 4 deep, for example.

varshasaha commented 5 years ago

@eps1lon Do i just need to add the cascading menu to the demo here? This comment has other tasks to it as well

mbrookes commented 5 years ago

@varshasaha Thanks for working on it. Neither of the CodeSandbox examples in this issue is fully functional, so there is more to do than just copying them to the docs.

The comment you linked to is a good starting point for understanding the objectives.

I would add two more to that list:

oliviertassinari commented 5 years ago

I agree. I have removed the docs and the good first issue tags.

FernandoGOT commented 5 years ago

Anyone have a working example of this? *With the last version of the package

ffjanhoeck commented 5 years ago

@FernandoGOT We already posted some examples. Or do you need something different?

mbrookes commented 5 years ago

@ffjanhoeck None of the examples posted is a complete working solution. Yours is closest, but still has a few issues:

It would be great to have a fully working solution for v4.

Additional context: https://github.com/mui-org/material-ui/issues/14345#issuecomment-458888753

IssueHuntBot commented 5 years ago

@gkiely has funded $20.00 to this issue.


gkiely commented 5 years ago

@varshasaha @mbrookes would love to see this implemented! I've got a few features depending on it.

mbrookes commented 5 years ago

So do you want to work on it? Happy to provide guidance where possible.

gkiely commented 5 years ago

Thanks Matt :) I can't this week but if it's still unsolved in a week or so I might give it a crack.

mbrookes commented 5 years ago

Pretty sure you will still find it available to work on, since no-one has solved it in the last two years! :)

If you make a start, and need someone to take a look, you could open a draft PR. Otherwise, if it's too early for a PR, push the branch to your repo, and link to it here (or PM on gitter or spectrum if you prefer).

rob2d commented 5 years ago

Related to this issue: would it be possible to expose placement (or something like PopoverProps) on the Menu component for the contained pop-over? Using either-or depending on whether implementing a menu or a sub menu is a little bit meh at this point with the @next branch. Doesn't solve things completely, but adds less overhead for users to implement themselves.

note: to clarify, I do realize you can use composition and thanks for the clear demo -- just think Menu should contain the idea of cascading menus so controlling things like where a Menu-in-Menu grows from is pretty important.

brandonferrer commented 5 years ago

Would love for the implementation to work with the Select API as well! :)

depiction commented 4 years ago

I'm in the process of upgrading a project from Material UI 0.19, which had a menu component that supported cascading menus, to version 4, which doesn't. I started with the demo that @janhoeck created and continued refining it. I copied the code that I ended up using to a Codepen for others to use. It addresses most of the concerns that everyone has mentioned.

The key was to use Menu for the root menu and the menu composition technique mentioned in the documentation.

https://codesandbox.io/s/material-ui-cascading-menu-wyyd0

oliviertassinari commented 4 years ago

@depiction Thanks for sharing, it doesn't completely solve the problem (e.g. keyboard), but it's a start, it can help waiting for us to fully support it :).

If you are interested in it, please upvote the issue. This is a metric we look at when prioritizing our work.

JeremyGrieshop commented 4 years ago

I'm in the process of upgrading a project from Material UI 0.19, which had a menu component that supported cascading menus, to version 4, which doesn't. I started with the demo that @janhoeck created and continued refining it. I copied the code that I ended up using to a Codepen for others to use. It addresses most of the concerns that everyone has mentioned.

The key was to use Menu for the root menu and the menu composition technique mentioned in the documentation.

https://codesandbox.io/s/material-ui-cascading-menu-wyyd0

Seems to work well for one level deep. Once you add a nested grandchild menu, it doesn't seem to work:

https://codesandbox.io/embed/material-ui-cascading-menu-fvsl4

ProfessorX737 commented 4 years ago

Hi I wrote a very simple NestedMenuItem component that works just like another MenuItem and can do infinitly nested menus. Each menu expands on hover. Demo: https://codesandbox.io/s/staging-wildflower-bzv19

sclavijo93 commented 4 years ago

@ProfessorX737 I've found a little bug in your demo.

ezgif com-video-to-gif

ProfessorX737 commented 4 years ago

@sclavijo93 Thanks for pointing that out! I fixed the issue now. Please let me know if there are any more bugs https://codesandbox.io/s/staging-wildflower-bzv19 I also added some css to highlight the cascading path

JeremyGrieshop commented 4 years ago

@ProfessorX737 Amazing demo, thanks working on this - a huge improvement over the default solutions and with not a lot of code!

I think the only thing that it needs is keyboard navigation.

azmenak commented 4 years ago

Recently needed something like this at d1g1t, so we built on @ProfessorX737's ideas.

Created https://github.com/azmenak/material-ui-nested-menu-item, you can see a demo here: https://codesandbox.io/s/material-ui-nested-menu-item-example-b25j6

It uses the basic concepts from the solution above and adds a few things:

We will be using this in production and will try to keep it up to date if we run into any bugs.

Lagily commented 4 years ago

@azmenak nice solution, when using the keyboard to navigate, it seems that the sub menus do not close when navigating away from an element

rtman commented 4 years ago

would be great if this could become official!

JeremyGrieshop commented 4 years ago

Recently needed something like this at d1g1t, so we built on @ProfessorX737's ideas.

Created https://github.com/azmenak/material-ui-nested-menu-item, you can see a demo here: https://codesandbox.io/s/material-ui-nested-menu-item-example-b25j6

It uses the basic concepts from the solution above and adds a few things:

  • Keyboard navigation (basic - allows going in and out of menus using left/right arrows in addition to the normal navigation)
  • Uses the theme from MUI for highlighting on hover
  • TS support with docs for each prop
  • Passthrough for ref and props

We will be using this in production and will try to keep it up to date if we run into any bugs.

This is the best one I've seen so far, congrats!

The one bug that I do see is w/r/t keyboard handling. If you use a mouse to move away from an expanded submenu, it works well, but if you use the keyboard to expand a menu, and keep navigating down with the keyboard, the original submenu stays active. To demonstrate/reproduce this on the codesandbox demo, simply do the following: Right Click to bring up the menu. Click the 'down' key twice so it highlights and expands 'Button 3' submenu. Now, click the 'down' key two more times to expand 'Button 5' submenu. You'll find that the 'Button 3' submenu is also still expanded.

ffjanhoeck commented 4 years ago

I have worked on a new example with some improvements aswell. Take a look at this sandbox: https://codesandbox.io/s/admiring-maxwell-etp7q

What do you think ?

rtman commented 4 years ago

@ffjanhoeck looking great, maybe its time to submit a PR?

JeremyGrieshop commented 4 years ago

What do you think ?

This works really well! When using it, my first instinct was to use Escape to back out of a menu and not LeftArrow. I can't seem to find any standardized documentation on how keyboard navigation should work on the web, but I've seen Escape used in other component frameworks, as well as Native (MacOS) to simply close out all menu options current open.

ProfessorX737 commented 3 years ago

Recently needed something like this at d1g1t, so we built on @ProfessorX737's ideas.

Created https://github.com/azmenak/material-ui-nested-menu-item, you can see a demo here: https://codesandbox.io/s/material-ui-nested-menu-item-example-b25j6

It uses the basic concepts from the solution above and adds a few things:

  • Keyboard navigation (basic - allows going in and out of menus using left/right arrows in addition to the normal navigation)
  • Uses the theme from MUI for highlighting on hover
  • TS support with docs for each prop
  • Passthrough for ref and props

We will be using this in production and will try to keep it up to date if we run into any bugs.

Hi so I took a look @azmenak improvements like keyboard navigation on my original example: https://codesandbox.io/s/material-ui-nested-menu-bzv19 But there were bugs with the key nav:

I've fixed these issues in v2: https://codesandbox.io/s/material-ui-nested-menu-v2-s7szu It now supports:

Future improvements:

I tested it quite a lot but let me know if there are any more bugs!