everweij / react-laag

Hooks to build things like tooltips, dropdown menu's and popovers in React
https://www.react-laag.com
MIT License
906 stars 44 forks source link

Support closeOnOutsideClick for nested menus #29

Closed mtmacdonald closed 4 years ago

mtmacdonald commented 4 years ago

Is it possible to make the closeOnOutsideClick setting work for nested popovers/menus?

I.e. if I have a nested menu, it should close when I click outside all of the menu layers, but not close the entire menu if I click inside a child layer (as it currently does).

If not, is there any workaround I can use?

Screenshot 2020-01-22 at 09 40 50
everweij commented 4 years ago

I agree this is something that needs to be fixed! Need sure how though, need to think about this for a moment 🤔

everweij commented 4 years ago

@mtmacdonald , I've added detection for nested layers inside the 'closeOnOutsideClick'-handling. By upgrading to v0.7.1 your issue should be fixed. Please, let me know if this works for you.

mtmacdonald commented 4 years ago

@everweij thanks a lot, that made a big a difference. It also resolved my related issue where sibling submenus could incorrectly be opened simulataneously, like so:

Screenshot 2020-02-10 at 17 18 15

I now wonder if onOutsideClick() could be similarly updated (or should that be a separate ticket)? When I tried a different approach of managing and setting isOpen myself, I tried to use onOutsideClick(), but it fires separately for each layer (i.e. it still fires for the other layers if I click inside one layer). Would it be possible to have something onOutsideAllClick() which fires only if you click outside all layers?

mtmacdonald commented 4 years ago

It seems that when closeOnOutsideClick is true, toggleLayerProps.isOpen is now always false? Which means that I can no longer close the menu by clicking on the trigger (only by clicking outside the menu).

mtmacdonald commented 4 years ago

Could we have a closeAll() handler? I'm calling close (e.g. upon clicking on an option) but I'm finding that it doesn't close all layers.

Screenshot 2020-02-10 at 19 29 30 Screenshot 2020-02-10 at 19 29 36
mtmacdonald commented 4 years ago

I've attached a minimum example of my remaining useToggleLayer issues:

npm install and npm run storybook to run, then see the 'with sub-menus story'.

menu.zip

everweij commented 4 years ago

Thanks @mtmacdonald for looking into this, very helpful 👍🏼 I will try some things out tomorrow. Will let you know where I will come up with.

everweij commented 4 years ago

Hi @mtmacdonald ,

I looked into the issues you were experiencing. Here are my findings:

toggleLayerProps.isOpen isn't working when closeOnOutsideClick is true (so I can't close the top-level menu by clicking on the trigger button. To reproduce try clicking the 'Menu' button to close the menu)

I found the cause of this bug, this should now be fixed in 1.7.2.

close doesn't allow me to close the entire menu when clicking an option (it closes some layers but not the root layer. To reproduce try clicking 'Area Chart')

I forgot to mention last weekend that by nesting multiple layers, the nested layers 'pass control' of the outside-clicking-behavior to the first layer in the tree that has closeOnOutsideClick=true. In most menu scenario's you have a root menu (with closeOnOutsideClick=true), and optional various sub-menu's (with closeOnOutsideClick=false).

As far as closing the entire menu from within a submenu is concerned: You could pass the close() of the root menu-layer down to the submenu's. So by closing the root menu, all other sub-menu's will unmount automatically as well.

I've updated your zip-file with a few lines of code. You can find them by searching for comments that start with 'Erik'

Let me know if this works for you :) menu (1.7.2).zip

everweij commented 4 years ago

Oh , by the way, I should probably document the nested closeOnOutsideClick stuff more thoroughly 😅

mtmacdonald commented 4 years ago

Thank you very much! I will check tomorrow and get back to you. Really appreciate the help.

From trying out your revised code, it looks those issues are solved, but now I'll need to handle closing of the individual sub-menu layers (since they no longer close when you click the sub-triggers). And it's also possible currently to open multiple sibling sub-menus. Hopefully from your comments I can figure that out myself in the morning.

mtmacdonald commented 4 years ago

I've had time to look at this in detail now and to get it working in a form I'm happy with. The main challenge I still had was preventing sibling nested menus from being open simultaneously. I managed to handle it by tracking all the close handlers of relevant layers so I could close sibling layers before opening a new submenu. Works but seemed like a lot of effort! Perhaps there's a better way, or it could be somehows included in react-laag in future.

I also stopped some event propagation so that for example, clicking on a heading in the layer wouldn't close the menu.

Apart from those points, your fixes solved my issues. Thanks again for all the help. I'm attaching the working solution in case its helpful for your future improvements, or inspiration for your examples/docs.

menu-working.zip

everweij commented 4 years ago

Hi @mtmacdonald, Good to hear you managed to get it to work!

I agree it's a tricky area to solve -> managing layers in a group setting. I think that in the future there should be some kind of mechanism which handles open / close behavior of these kind of related toggle-layers (like nested menu's). Right now, I think I rather put this logic inside an own hook or component, because I want to limit the complexity and responsibilities that ToggleLayer / useToggleLayer currently has.

Thanks for sharing your code, helpful stuff 👍🏼