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

Make the snippet preview modal content always scrollable #10832

Merged
merged 1 commit into from
Sep 3, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Aug 29, 2018

Summary

This PR can be summarized in the following changelog entry:

  • N/A

Relevant technical choices:

This PR restores the plugin modal scrolling when the viewport height is smaller than the modal height.
Previously, the modal used an inline style set via the component style prop:

style={ { height: "initial", minHeight: "50px" } }

The modal content uses overflow: auto to make its content scrollable but the scrolling mechanism needs a computed height to work. It doesn't work with height: initial or height: auto so this inline style is not a viable option. The PR removes this inline style. Now the modal content is scrollable again. The downside is that on big screens the modal height won't be based on its content height any longer and it will be the default height of the Gutenberg component (70%). This should be improved upstream and there's already an issue for that. I've also explored a possible solution, will push a PR soon.

Test instructions

This PR can be tested by following these steps:

  • first, reproduce the bug on master:
  • use the Chrome device emulator and set it to iPhone 6/7/8 or iPhone 6/7/8 Plus in landscape orientation
  • edit a post and open the snippet preview modal from the plugin sidebar
  • verify the modal content is not scrollable
  • switch to this branch and build the JS
  • test again and verify the modal content is now scrollable

As said, on big screens the modal won't extend his height to the content height and will be limited to 70%, see in the screenshot below:

screen shot 2018-08-29 at 08 53 56

Fixes #10811
Fixes #10703

@afercia afercia changed the title Remove inline style. Make the snippet preview modal content always scrollable Aug 29, 2018
@afercia
Copy link
Contributor Author

afercia commented Aug 31, 2018

Seems waffle doesn't want to connect this PR to #10703.
Fixes #10703

@Yoast Yoast deleted a comment from afercia Sep 3, 2018
@Yoast Yoast deleted a comment from afercia Sep 3, 2018
@moorscode
Copy link
Contributor

You need to place the Fixes into the main content, not as a comment.

@boblinthorst
Copy link
Contributor

CR: ok 👍

@boblinthorst
Copy link
Contributor

Acceptance: Works as intended 👍

@boblinthorst boblinthorst added this to the 8.3 milestone Sep 3, 2018
@boblinthorst boblinthorst merged commit 5f7d371 into trunk Sep 3, 2018
@boblinthorst boblinthorst deleted the 10811-snippet-preview-modal-scrollable branch September 3, 2018 12:35
@afercia
Copy link
Contributor Author

afercia commented Sep 17, 2018

For reference: I've pushed a PR upstream to propose a new approach for the modal height, see WordPress/gutenberg#9973

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

Successfully merging this pull request may close these issues.

3 participants