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

[Popover] Don't hide scrollbar #18898

Open
1 task done
lcswillems opened this issue Dec 17, 2019 · 6 comments
Open
1 task done

[Popover] Don't hide scrollbar #18898

lcswillems opened this issue Dec 17, 2019 · 6 comments
Labels
component: Popover The React component. new feature New feature or request priority: important This change can make a difference

Comments

@lcswillems
Copy link
Contributor

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

In the documentation, it is written: "The scroll and click away are blocked unlike with the Popper component.". But why? Why not allowing to unlock the scrollbar as it is the case for Select, Menu, Dialog, etc...?

You will say me: "use the popper". I can do it, but the popper misses a lot of features that the popover has (escape key & click away).

Moreover, Google alreadys uses Popover that doesn't block the scrollbar:

image

So it would be nice if there is a way to not block the scrollbar on Popover (this is the only that prevents me to use it everytime I want to use it).

@lcswillems lcswillems changed the title Allow to unlock scrollbar for Popover Don't hide scrollbar for Popover Dec 17, 2019
@lcswillems lcswillems changed the title Don't hide scrollbar for Popover [Popover] Don't hide scrollbar Dec 17, 2019
@lcswillems
Copy link
Contributor Author

lcswillems commented Dec 17, 2019

After investigation, Google prevents scrolling when subscribe popover is opened without hiding the scrollbar.

I think they do something like:

useEffect(() => {
  const preventScrolling = e => e.preventDefault()

  window.addEventListener('scroll', preventScrolling);
  return () => window.removeEventListener('scroll', preventScrolling);
}, []);

@oliviertassinari oliviertassinari added the duplicate This issue or pull request already exists label Dec 17, 2019
@oliviertassinari
Copy link
Member

We have started a discussion around a potential solution to this problem in #17636 (comment)

The Popover behaves like a modal: it changes the active frame to the content of the component. However, we have seen people ask for option to opt-out some of this modal behavior: #11243 and #17353.

Regarding Google's solution, does it work with the mouse wheel, does it work manipulation the scroll bar, does it work on mobile, does it work with scrollable content in the Popover?

@oliviertassinari
Copy link
Member

You can implement click away with the ClickAwayListener. I don't think that we need to keep this issue open, I overlap with the one I have linked. Right now, you can use disableScrollLock but positioning won't be right, which is why I was proposing to use the Popper component in the Popover.

@nikossoftwaredev
Copy link

Use disableScrollLock prop and with Styles
root: {
position: 'absolute !important',
},

@oliviertassinari oliviertassinari added component: Popover The React component. and removed duplicate This issue or pull request already exists labels Jul 9, 2020
@oliviertassinari oliviertassinari added this to the v5 milestone Jul 9, 2020
@oliviertassinari
Copy link
Member

I can't find the direct duplicate, let's keep it open so we don't forget.

dalegacusan pushed a commit to dalegacusan/rift-builds that referenced this issue Mar 26, 2021
1. Changed UI for Copy Build Link

2. Added a "View more builds for this champion" button in Player Build page

3. Fixed FAQ Questions and Answers

4. Used "disableScrollLock" for Item, Rune, and Spell Popovers because the default behavior of Material UI popover is to disable scrolling while a popover is open (reference: mui/material-ui#18898)
@oliviertassinari oliviertassinari modified the milestones: v5-beta, v5.1 May 10, 2021
@oliviertassinari oliviertassinari added new feature New feature or request priority: important This change can make a difference labels May 10, 2021
@oliviertassinari oliviertassinari removed this from the v5.1 milestone Nov 10, 2021
@BierDav
Copy link

BierDav commented Aug 22, 2023

After investigation, Google prevents scrolling when subscribe popover is opened without hiding the scrollbar.

I think they do something like:

useEffect(() => {
  const preventScrolling = e => e.preventDefault()

  window.addEventListener('scroll', preventScrolling);
  return () => window.removeEventListener('scroll', preventScrolling);
}, []);

i would appreciate a prop that lock scrolling similar to this or this, because when the scrollbar hides and appears again, it changes the view width, which results in unexpected flickering for the user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popover The React component. new feature New feature or request priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests

4 participants