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

Reveal multiple opened: body scrolls top #11704

Closed
3 tasks done
kolejakjozef opened this issue Feb 28, 2019 · 5 comments · Fixed by #11705
Closed
3 tasks done

Reveal multiple opened: body scrolls top #11704

kolejakjozef opened this issue Feb 28, 2019 · 5 comments · Fixed by #11705

Comments

@kolejakjozef
Copy link

kolejakjozef commented Feb 28, 2019

What should happen?

When using multiple opened reveals, the html position scroll shoudn't change on reveal open

What happens instead?

The second and every other opened reveal sets html css top value to 0 and page jumps top

Possible Solution

If you allready have one reveal opened, the next one shoudn't call _disableScroll() and on close it shoudn't call _enableScroll()

This simple fix should help

foundation.reveal.js, change line 447 to:

if ($('.reveal:visible').length === 0) {
    _this._enableScroll(scrollTop);
}

foundation.reveal.js, change line 274 to:

if ($('.reveal:visible').length === 0) {
  this._disableScroll();
}

Test Case and/or Steps to Reproduce (for bugs)

Test Case: https://codepen.io/anon/pen/WmQXvN
Reveal open button is on the bottom of the page

How to reproduce:

  1. open first reveal by clicking the button Click me for a modal
  2. open second reveal by clicking the button inside first reveal
  3. page jumps top

Your Environment

  • Foundation version(s) used: 6.5.3
  • Browser(s) name and version(s): Chrome 72.0.3626.119
  • Device, Operating System and version: Windows 10 PC

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • There are no other issues similar to this one.
  • The issue title and template are correctly filled.
@DanielRuf
Copy link
Contributor

  • Foundation version(s) used: 6.5.3

Here is an updated codepen with 6.5.3: https://codepen.io/DanielRuf/pen/moeXEB

Your codepen uses 6.5.1

@DanielRuf
Copy link
Contributor

Can you test #11705? This would be great.

@kolejakjozef
Copy link
Author

Tested on my side and aprooved #11705
Thank you :)

@kolejakjozef
Copy link
Author

@DanielRuf btw, should i close this issue ? Sorry i'm new here, this was my first issue.

@DanielRuf
Copy link
Contributor

@DanielRuf btw, should i close this issue ? Sorry i'm new here, this was my first issue.

You did nothing wrong. Thanks for creating the issue and the feedback.
We have to wait now for a review by @ncoden to see how and when we merge it.

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

Successfully merging a pull request may close this issue.

2 participants