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.

Sign up for GitHub

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

Jump to bottom

enableBodyScroll doesn't enable scroll #90

Closed
An-Cu opened this issue Jan 28, 2019 · 12 comments
Closed

enableBodyScroll doesn't enable scroll #90

An-Cu opened this issue Jan 28, 2019 · 12 comments

Comments

@An-Cu
Copy link
Contributor

An-Cu commented Jan 28, 2019 •
edited

enableBodyScroll doesn't enable scroll in situation disableBodyScroll was required (n+1)-times and enableBodyScroll n-times.
enableBodyScroll and disableBodyScroll was required without arguments.

To reproduce the issue try the next sequence of function requiring:
disableBodyScroll()
disableBodyScroll()
enableBodyScroll()
as result overflow: hidden styles in <body> still persist.

I already have prepared commit to fix this issue
An-Cu pushed a commit to An-Cu/body-scroll-lock that referenced this issue Jan 28, 2019
willmcpo#90 fix not worked enableBodyScroll in some conditions
3eae730
@An-Cu An-Cu mentioned this issue Jan 28, 2019
Merged
@hacknug
Copy link

hacknug commented May 6, 2019

@willmcpo anything I can do to help get the PR merged? You can replicate the issue by clicking twice disableBodyScroll and then enableBodyScroll on your demo link.

@SeanSilke
Copy link

The same with me. I have modal inside modal. And this line evaluated too false because locks length is > 1.

@diachedelic
Copy link
Collaborator

@SeanSilke if you close one modal and still have one open, the body should still be locked, right?

@SeanSilke
Copy link

@diachedelic Yes. My problem is with unlocking body scroll.

@willmcpo
Copy link
Owner

i started reviewing this one

@willmcpo
Copy link
Owner

Why would you ever call disableScroll on the same target element?

@kossel
Copy link

kossel commented Jun 16, 2019

I have similar problem but when the targetElement doesn't exist anymore, eg. it unmounted or something the body's overflow would never be removed.

@willmcpo
Copy link
Owner

willmcpo commented Jun 16, 2019 via email

@kossel
Copy link

kossel commented Jun 16, 2019 •
edited

yes, please see this codesandbox I created to reproduce it.

it's a very very slim down version of what I'm doing, basically for modal, we offer the option of "unmount/destroy content when close" so when the isOpen become false, the scroll target element is not longer exits,
and this line returns false
https://github.com/willmcpo/body-scroll-lock/blob/master/src/bodyScrollLock.js#L232
since targetElement is null.

@SeanSilke
Copy link

@willmcpo wrote Why would you ever call disableScroll on the same target element?
In our case we call for popup from popup.
The disableScroll mast be Idempotent, number of calls should not affect result.

@willmcpo
Copy link
Owner

@SeanSilke

Sure. The pull request to fix this needs to make an amendment then we can merge.

An-Cu added a commit to An-Cu/body-scroll-lock that referenced this issue Jun 27, 2019
@An-Cu
willmcpo#90 fix not worked enableBodyScroll in some conditions
a95516a
An-Cu added a commit to An-Cu/body-scroll-lock that referenced this issue Jun 27, 2019
@An-Cu
willmcpo#90 fix not worked enableBodyScroll in some conditions
d1b8b07
willmcpo added a commit that referenced this issue Jun 27, 2019
@willmcpo
Merge pull request #91 from An-Cu/master
f31fa4b 
#90 fix not worked enableBodyScroll in some conditions
@willmcpo
Copy link
Owner

Merged fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@diachedelic @SeanSilke @hacknug @kossel @willmcpo @An-Cu