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 horizontal scrollbar on full-wide blocks with nesting. #16085

Merged
merged 1 commit into from
Jun 12, 2019

Conversation

jasmussen
Copy link
Contributor

This fixes #15192.

This PR fixes two issues:

  1. There was an issue, probably rebase related, where the columns block had CSS to prevent horizontal scrollbars when fullwide, but which didn't work anymore. I simply fixed the selector again.
  2. The recent merge of the clickthrough PR failed to take into account fullwide blocks with nesting, and caused a horizontal scrollbar due to the overlay extending beyond the canvas.

To test this PR, please verify that full wide alignments work as intended. You can test columns, image, embed, media & text.

Please also verify that blocks with nesting work as intended, both in fullwide and not wide situations.

Before:

before

After:

after

This fixes #15192.

This PR fixes two issues:

1. There was an issue, probably rebase related, where the columns block had CSS to prevent horizontal scrollbars when fullwide, but which didn't work anymore. I simply fixed the selector again.
2. The recent merge of the clickthrough PR failed to take into account fullwide blocks with nesting, and caused a horizontal scrollbar due to the overlay extending beyond the canvas.

To test this PR, please verify that full wide alignments work as intended. You can test columns, image, embed, media & text.

Please also verify that blocks with nesting work as intended, both in fullwide and not wide situations.
@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Jun 11, 2019
@jasmussen jasmussen added this to the Gutenberg 5.10 milestone Jun 11, 2019
@jasmussen jasmussen requested review from gziolo and kjellr June 11, 2019 08:01
@jasmussen jasmussen self-assigned this Jun 11, 2019
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Works well on my end. Thanks for fixing this, @jasmussen!

Before

Screen Shot 2019-06-11 at 6 34 33 AM

gutenberg test_wp-admin_post php_post=1 action=edit(iPhone X)

After

Screen Shot 2019-06-11 at 6 36 46 AM

gutenberg test_wp-admin_post php_post=1 action=edit(iPhone X) (1)

Note that this does reinstate the extra left/right margins for columns. We may end up wanting to go back and revert/update this change for Twenty Nineteen. It turns out this change was prompted by the bug that's fixed here. 😄 https://core.trac.wordpress.org/ticket/46643

Revising that approach would eliminate this bug:
https://core.trac.wordpress.org/ticket/47044

@gziolo gziolo removed this from the Gutenberg 5.10 milestone Jun 11, 2019
@jasmussen
Copy link
Contributor Author

Note that this does reinstate the extra left/right margins for columns. We may end up wanting to go back and revert/update this change for Twenty Nineteen. It turns out this change was prompted by the bug that's fixed here. 😄 https://core.trac.wordpress.org/ticket/46643

I'm sorry that I didn't fix this before so that TwentyNineteen aligned i the wrong direction.

The reason why there is extra margin is actually in absence of a better solution. As is noted also in the code comment:

// Fullwide: show margin left/right to ensure there's room for the side UI.
// This is not a 1:1 preview with the front-end where these margins would presumably be zero.

In other words — we should look at a better solution, but do that holistically. Perhaps with the recent improvements to selecting nested blocks, and your incoming outlines PR, maybe now we have the tools to do so? Maybe in a way that's the same for fullwide images and fullwide columns?

In the mean time, I don't have a strong opinion on changing TwentyNineteen. If we have a great idea for how to improve this in Gutenberg and believe we can ship it faster, maybe it's fine?

Thanks for the review!

@jasmussen jasmussen merged commit af57dc9 into master Jun 12, 2019
@jasmussen jasmussen deleted the fix/columns-full-width branch June 12, 2019 09:10
@kjellr
Copy link
Contributor

kjellr commented Jun 12, 2019

In the mean time, I don't have a strong opinion on changing TwentyNineteen. If we have a great idea for how to improve this in Gutenberg and believe we can ship it faster, maybe it's fine?

Agreed. Let's leave for now. Once the nesting stuff gets finalized we can re-assess the extra padding we currently have on columns. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Columns Block: Full-width causes horizontal scroll.
4 participants