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

bindHover on menu #3

Closed
RZsam opened this issue Jan 13, 2019 · 20 comments
Closed

bindHover on menu #3

RZsam opened this issue Jan 13, 2019 · 20 comments

Comments

@RZsam
Copy link

RZsam commented Jan 13, 2019

hey there
I'm trying to use bindHover with bindMenu but when mouse moves on sub menu list is closed...
am I doing something wrong ?
if not I think it's good feature to add

                        {
                            popupState => (
                                <React.Fragment>
                                    <MenuItem
                                        className={classes.menuItem}
                                        {...bindHover(popupState)}>
                                        {item.title}
                                    </MenuItem>
                                    <Menu
                                        anchorOrigin={{
                                            vertical: "bottom",
                                            horizontal: "right"
                                        }}
                                        className={classes.popover}
                                        getContentAnchorEl={null}
                                        TransitionComponent={Fade}
                                        {...bindMenu(popupState)}
                                    >
                                        elevation={1}
                                        >
                                        {
                                            item.subMenu.map((sub, j) =>
                                                <NavLink to={sub.path} key={j}>
                                                    <MenuItem
                                                        className={classes.subMenu}
                                                    >
                                                        {sub.title}
                                                    </MenuItem>
                                                </NavLink>
                                            )}
                                    </Menu>
                                </React.Fragment>
                            )
                        }
                    </PopupState>```
@RZsam RZsam changed the title popover on menu bindHover on menu Jan 13, 2019
@jedwards1211
Copy link
Member

Sorry for the delay, GitHub hadn't added me as a watcher of this package :(

You're not doing anything wrong, but bindHover was designed to close the popover whenever the bound component is no longer being hovered.

I'll try to solve this by checking in onMouseLeave if event.relatedTarget is a descendant of the menu.

@jedwards1211
Copy link
Member

Okay, I released changes that should fix this in version 1.1.0. Can you install version 1.1.0 and let me know if it fixes your issue?

Note: the fix only works if you pass a popupId prop to PopupState, because it needs popupId to find your submenu component.

Also, if there is any space between the menu item and the submenu, it could close the submenu if the mouse is not moved quickly enough for the cursor to jump straight from the menu item to the submenu. One potential solution to this would be (if possible) put the menu item and submenu inside of a <div> or <span>, and move bindHover to that enclosing <div> or <span> instead of the menu item.

@RZsam
Copy link
Author

RZsam commented Jan 29, 2019

@jedwards1211
you're right about bindHover behavior but I think it would be a good feature if you can choose whether it closes or not.
I've tried again with version 1.1.0 and it's working. thank you!
but found another issue :
when the cursor leaves MenuItem menu is still open. again I think it is a good prop to add if menu remains open or it closes after mouseLeave.

@jedwards1211
Copy link
Member

Okay either I need to refine my tests or try this out myself in a browser, the logic I added was supposed to close when the menu item or the submenu gets a mouseleave where the mouse has left to something that's not a descendent of either two. Definitely didn't intend for it to be stuck open after you mouse away from either :)

@RZsam
Copy link
Author

RZsam commented Jan 29, 2019

@jedwards1211 let me know when new version released...
please perform a test yourself maybe I'm missing something!

@jedwards1211
Copy link
Member

Okay, the problem is that the menu I get by id is actually the container element that fills up the whole window, rather than the actual menu element. I'll figure out how to solve this...

@jedwards1211
Copy link
Member

jedwards1211 commented Jan 30, 2019

@Linton-Samuel-Dawson unfortunately it's pretty much impossible to detect if the mouse is over the trigger element in any clean way, because of the way Popover and Menu use Modal, which spams a big window-filling element to the document root, blocking pointer events. If I try to disable pointer events to that element so I can tell if the trigger element is being hovered, it prevents the menu from receiving any pointer events. I'm really angry at the Material UI team about this and I've complained loudly in their issue tracker about it.

@jedwards1211
Copy link
Member

@Linton-Samuel-Dawson I'm sorry you've had trouble with this and that I haven't been able to figure out a way to make it work for you yet. I'm sure it's been frustrating for you too

@RZsam
Copy link
Author

RZsam commented Jan 31, 2019

@jedwards1211 thank you for effort ...
we will wait for material ui team I think

@schemenauero
Copy link

schemenauero commented Feb 13, 2019

I got this working by using a popper (which doesn't have a backdrop) and putting the popper inside the button component. It looks like this (using hooks):

import React from 'react'
import { usePopupState, bindPopper } from 'material-ui-popup-state/hooks'
import { bindHover } from 'material-ui-popup-state'

import Button from '@material-ui/core/Button'
import Popper from '@material-ui/core/Popper'
import Paper from '@material-ui/core/Paper'
import Typography from '@material-ui/core/Typography'

export const ToolbarMenu = props => {
  const popupState = usePopupState({ variant: 'popover', popupId: 'demoId' })

  return <Button
    {...bindHover(popupState)}
    onMouseLeave={popupState.close}
  >
    Hover Here ▼
    <Popper {...bindPopper(popupState)}>
      <Paper>
        <Typography> Menu Item 1 </Typography>
        <Typography> Menu Item 2 </Typography>
      </Paper>
    </Popper>
  </Button>
}

Also had to add an onMouseLeave to the button. Doesn't look like onMouseLeave is working with bindHover (which isn't in the hooks file).

@jedwards1211
Copy link
Member

jedwards1211 commented Feb 13, 2019

@schemenauero wow, you figured that out fast, considering I released the hooks API last night! It's funny too that you worked around the fact that I didn't add bindHover to the hooks code yet.
I feel on the fence about whether to add bindHover and some kind of onMouseLeave handlers to the hooks API yet...what do you think? I'm hoping the Material UI team will consider substantial changes to their modal and menu components...

@jedwards1211
Copy link
Member

jedwards1211 commented Feb 13, 2019

@schemenauero note also that @Linton-Samuel-Dawson is wanting the Menu to stay open when the mouse moves from the button to the Menu, so to solve OP issue it isn't sufficient to put onMouseLeave={popupState.close} on the button.
I just realized, putting the Popper within the button makes it so that no mouseleave is fired when the mouse is moved over the Popper, right?

@schemenauero
Copy link

schemenauero commented Feb 14, 2019

@jedwards1211 Yea, the mouseleave fires when it leaves the button/popper combo, so it works like @Linton-Samuel-Dawson desires I think. I had a problem where, when the Button had an onClick, it was overriding any components with onClick's in the popper (because the popper was the parent), but I fixed that by using a Link instead of a Button. A bit "hacky" to get it all to work, but it makes sense.

A prop allowing us to disable the backdrop of Menu's would be nice, but I can't speak towards Material UI's priorities. I think it would be nice to either add bindHover to the hooks API so it works the same as the normal API or to reflect its absence in the documentation.

The hooks API is great by the way, thank you! Allowed me to write cleaner code in fewer lines.

@jedwards1211
Copy link
Member

jedwards1211 commented Feb 15, 2019

You're welcome!

To be perfectly clear, there is not just a backdrop, but also a modal root element enclosing the backdrop and the popover. It renders like this:

<div> (modal root)
  <div /> (backdrop)
  <div> (Paper)
    (popover content)
  </div>
</div>

Popover hides the backdrop element, but there is currently no getting rid of the modal root element and mounting the Paper element by itself. So one has to make the modal root invisible to the mouse while keeping the paper visible to the mouse.

I figured out that the following styles on the Popover will accomplish what we need with hover interaction:

const styles = {
  popover: {
    pointerEvents: 'none',
  },
  paper: {
    pointerEvents: 'auto',
  },
}

...
        <Popover
          {...bindPopover(popupState)}
          className={classes.popover}
          classes={{
            paper: classes.paper,
          }}

I don't want you guys to have to write these styles everywhere you use hover interaction though. I guess the best solution is to have this library export a Popover wrapped with these styles for convenience (as well as a Menu).

@jedwards1211
Copy link
Member

@Linton-Samuel-Dawson @schemenauero okay guys, I've almost solved this, haven't published yet but I have a demo up!
https://jcoreio.github.io/material-ui-popup-state/#cascading-hover-menus

I'm working on this in the cascading-hover branch.
Still need to clean up a few more things, for instance I sometimes see warnings in the console about a setState (hook) call on an unmounted component.

@jedwards1211
Copy link
Member

jedwards1211 commented Feb 16, 2019

@schemenauero btw, in this branch the hooks and render props APIs now share code under the hood and the hooks API supports bindHover 💪

@grachet
Copy link

grachet commented Mar 7, 2019

Hy,

can you push your code to the master branch please if you ended it ?

@jedwards1211
Copy link
Member

Yeah I'll work on it, a few more things need to ne cleaned up

@grachet
Copy link

grachet commented Mar 7, 2019

ok thanks

@jedwards1211
Copy link
Member

Okay this is released in 1.3.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants