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

fix: keep a scrollbar on document when Reveal opens #7831 #11341

Merged
merged 4 commits into from
Jun 19, 2018

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Jun 17, 2018

Description

#11065 was intented to avoid double scrollbars from a Reveal modal, but removing all scrollbars when the modal opens changes the page content width and the content shift. This is a regression of #7831.

The current solution is a mix between the two previous fixes:

  • Modal may have a scrollbar or not, following its content height
  • Body has no scrollbar and prevent any scroll
  • Document may have a scrollbar, following its content height (controlled in JS)

So the body content never shift, as the document has a scrollbar when a Reveal modal is open only if it already had one before.

Changes:

  • add the zf-has-scroll global html modifer and toggle it on Reveal opening/closing and on window resizing
  • always prevent scroll on body instead of html tag like in fixed issue #7831 #10583

foundation#11065 was intented to avoid double scrollbars from a Reveal modal, but removing all scrollbars when the modal opens changes the page content width and the content shift. This is a regression of foundation#7831.

The current solution is a mix between the two previous fixes:
* Modal may have a scrollbar or not, following its content height
* Body has no scrollbar and prevent any scroll
* Document may have a scrollbar, following its content height (controlled in JS)

Changes:
* add the `zf-has-scroll` global html modifer and toggle it on Reveal opening/closing
* always prevent scroll on body instead of html tag like in foundation#10583

See foundation#10791 (comment)
Closes foundation#7831
@ncoden
Copy link
Contributor Author

ncoden commented Jun 17, 2018

Poke @isapir for review

@ncoden ncoden added this to the 6.5.0 milestone Jun 17, 2018
@DanielRuf
Copy link
Contributor

This does probably not respect viewport changes.

@DanielRuf
Copy link
Contributor

Poke @isapir for review

Was the review request meant for @isapir or me?

@ncoden
Copy link
Contributor Author

ncoden commented Jun 17, 2018

@DanielRuf Both

@ncoden
Copy link
Contributor Author

ncoden commented Jun 17, 2018

This does probably not respect viewport changes.

You're right, we should watch for viewport changes. I don't like doing things like this in JS but this seems to be the only solution.

@ncoden
Copy link
Contributor Author

ncoden commented Jun 17, 2018

Some comments to describe that "disabled scrollbar" behavior would be useful tho.

Copy link
Contributor

@isapir isapir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ncoden @DanielRuf It looks fine to me though I didn't test it by running.

IMO in the edge case that the viewport changes while the Reveal is open then it's OK to show a slight shift and/or disabled scrollbar.

@ncoden
Copy link
Contributor Author

ncoden commented Jun 18, 2018

@isapir @DanielRuf I made the requested changes and some refactor.

82a0493 fix: show/hide Reveal scrollbar on window resizing
deda9dc refactor: rename Reveal internal methods for a better clarity
96141b7 docs: add doc for Reveal methods _addGlobalClasses/_removeGlobalClasses

@DanielRuf
Copy link
Contributor

fix: show/hide Reveal scrollbar on window resizing

I guess orientationchange is missing.

@ncoden
Copy link
Contributor Author

ncoden commented Jun 18, 2018

I guess orientationchange is missing.

Doesn't it count as a resize ?

@ncoden
Copy link
Contributor Author

ncoden commented Jun 18, 2018

I got my answer https://stackoverflow.com/a/23996839/4317384

Some devices/browsers do, some not.

Web development often sounds like biology courses. There's rule for everything and exception for every rule.

@ncoden
Copy link
Contributor Author

ncoden commented Jun 19, 2018

@DanielRuf Actually I am using resizeme from utils.triggers, and the entire framework rely on this. So if we decide to use it (according to browser support), we'll not do it in this PR.

@DanielRuf
Copy link
Contributor

Ok.

@ncoden ncoden merged commit fb9a534 into foundation:develop Jun 19, 2018
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 20, 2018
…lbar-7831 for v6.5.0

96141b7 docs: add doc for Reveal methods _addGlobalClasses/_removeGlobalClasses
deda9dc refactor: rename Reveal internal methods for a better clarity
82a0493 fix: show/hide Reveal scrollbar on window resizing
e6eb9b0 fix: keep a scrollbar on document when Reveal opens foundation#7831

Signed-off-by: Nicolas Coden <[email protected]>
@ncoden ncoden mentioned this pull request Jun 20, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants