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

Try: Remove/refactor complex left/right block margins #17877

Merged
merged 1 commit into from Nov 7, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Oct 10, 2019

Every block, specifically .block-editor-block-list__block, has had paddings left and right to increase the hoverable area to invoke the up/down mover. This padding is then compensated for by a deeper container with negative left and right margins.

Since the mover control is now only visible on-select, those negative margins are no longer necessary.

This PR refactors/removes those negative margins. This drastically simplifies the CSS of the block list component, and should make it easier for themes to get good results when they style the editor.

This is a work in progress. When done it needs to not land in 5.3, and it will need a dev-note when ready because unfortunately it is likely to break a number of existing editor styles. For example it breaks the off-set column and wide/full-wide styles in TwentyNineteen. The good news is that it should be easy to fix by removing a ton of hacky TwentyNineteen editor CSS.

Things to test when the PR becomes ready for review:

  • Test primarily and first on a theme that does not style the editor.
  • Test master and this branch and both should look identical.
  • Test wide and fullwide and look for no horizontal scrollbars.
  • Test every block that features nesting, notably Cover, Columns, Group.

While this PR is ready for review, it needs a lot of testing, and given its post-5.3 destination, I wouldn't call it urgent.

However the code quality is drastically improved, and there's lots of red to be found, so it's definitely worth it.

Copy link
Contributor

youknowriad left a comment

Nice PR, seems to work well but when I tried the widgets screen, it was empty. I wonder if a rebase should fix it or maybe there's something else?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Oct 24, 2019

I'll try a rebase. Given it's CSS only (albeit quite extensive CSS), it shouldn't make the widget screen break.

@jasmussen jasmussen force-pushed the try/refactor-horizontal-margins branch from 10fd31d to 6acfdd2 Oct 24, 2019
Copy link
Contributor

youknowriad left a comment

LGTM

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Oct 24, 2019

Wow, that went easier than I'd expected :)

Thank you for the review. I think I'll still want a few more eyes on it — it is unfortunately likely to require quite a few editor styles to need adjustments.

@jasmussen jasmussen force-pushed the try/refactor-horizontal-margins branch from 6acfdd2 to 1d6548a Oct 29, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Oct 29, 2019

Rebased. One of the reasons this needs more eyes is that it does affect editor styles. Here's TwentyNineteen:

Screenshot 2019-10-29 at 14 43 36

I'll try and prepare a patch for that, shouldn't be hard. But it's little things where the previous left and right margins were compensated for, in a way that's no longer necessary.

@mapk
mapk approved these changes Oct 30, 2019
Copy link
Contributor

mapk left a comment

Looks good. I tested on FF with several blocks.

I used to see extra padding in the Inspector like this:

Screen Shot 2019-10-30 at 8 24 10 AM

But now it all looks good! Thanks for simplifying this!

padding

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Oct 30, 2019

Thanks Mark for testing, and thanks everyone else for also testing. It's important to get many eyes on this, not just to test that it's good, but also to raise awareness of the change happening, so that if/when subsequent issues pop up, there's a chance you know why they're happening and where they originated :)

Anyone know why the tests are failing? Given the depth of the changes it could definitely be legitimate failures, so would appreciate insights by anyone who has the time to look.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Oct 30, 2019

Suggested dev note:

In <a href="https://github.com/WordPress/gutenberg/pull/17877">17877</a> a change was made to how blocks are laid out in the block editor. Previously, every block would come with padding built in, left and right, and negative margins to compensate. These margins and paddings have been refactored away. This should drastically simplify the CSS necessary to style blocks, for block developers and for WordPress themers wanting to style the editor.

The refactor may cause issues in existing block or editor styles that have compensated for the previous margins/paddings. If your block or editor style looks off, a likely fix is to remove the styles used to compensate.
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Oct 31, 2019

I think the test failure is unrelated, I restarted the job to see.

@jasmussen jasmussen force-pushed the try/refactor-horizontal-margins branch 2 times, most recently from 61864f9 to 4c4ef90 Nov 1, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 1, 2019

Rebased and squashed, just to keep it simpler to shepard along.

I've also tested image floats, and they work well here.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 1, 2019

I restored a rule for the title block that I had deemed unnecessary. But resting it fixes the issue with TwentyNineteen, and therefore very probably a ton of other themes, so in doing so it makes this PR more resilient and less risky.

I also included a todo in the CSS: c104127 — we will revisit that area regardless if/when the Title becomes a block.

I did a bunch of testing in themes, and it's looking good:

Astra:

Screenshot 2019-11-01 at 10 15 02

Vanilla:

Screenshot 2019-11-01 at 10 15 19

A bunch of the Twenty themes:

Screenshot 2019-11-01 at 10 15 30

Screenshot 2019-11-01 at 10 15 43

Screenshot 2019-11-01 at 10 15 54

Screenshot 2019-11-01 at 10 16 32

TwentyTwenty, notably, looks delightful:

Screenshot 2019-11-01 at 10 17 32

With all this, I'm feeling much more calm about merging this. It still needs a dev note, and it would still be good to go out in the next Gutenberg plugin so themers and block devs can be aware of this early.

@jasmussen jasmussen force-pushed the try/refactor-horizontal-margins branch 2 times, most recently from 79de22e to a83b49d Nov 4, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 4, 2019

Rebased, squashed, and would appreciate insights on when's a good time to merge this.

For the record I'm seeing two issues this PR has caused for TwentyNineteen:

The line before the Title isn't flush:
Screenshot 2019-11-04 at 08 48 08

Fullwide is broken:
Screenshot 2019-11-04 at 08 48 16

This is only in TwentyNineteen, not in any of the other ones, and it's becasue that theme compensated for the margins this PR removes. I will push a PR to twentyninteen once this one is merged.

Those margins were created to accommodate a mover control that was available on hover. Given the mover control is now on-select only, this margin is no longer necessary.

This is a work in progress. When done it needs to not land in 5.3, and it will need a dev-note when ready.
@jasmussen jasmussen force-pushed the try/refactor-horizontal-margins branch from a83b49d to 910fd0d Nov 6, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 6, 2019

Rebased this again, also to see if that makes the tests pass. If not, I could use some help to figure out if there's a legitimate test failure. @gziolo if you have time?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 6, 2019

Rebased this again, also to see if that makes the tests pass. If not, I could use some help to figure out if there's a legitimate test failure. @gziolo if you have time?

Travis seems to be happy about your work. It should be given that this PR adds only visual changes applied through CSS styles. I guess this is good to go based on the previous approvals.

@gziolo gziolo removed the [Type] RFC label Nov 6, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 6, 2019

Woop! I guess it's merge time.

It will require a patch to TwentyNineteen, so I'll probably put this on my schedule for tomorrow, merge then patch twentynineteen. Thanks!

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 7, 2019

I have created https://core.trac.wordpress.org/ticket/48526 to address the issues introduced in TwentyNineteen by this patch.

The dev note for this patch is in #17877 (comment).

I'm merging now and hopefully we can get the core patch in shortly after 5.3.

@jasmussen jasmussen merged commit 03b1e80 into master Nov 7, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the try/refactor-horizontal-margins branch Nov 7, 2019
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
width: 100%;
}
margin-left: -$block-padding - $block-padding - $block-side-ui-width - $border-width - $border-width;
margin-right: -$block-padding - $block-padding - $block-side-ui-width - $border-width - $border-width;

This comment has been minimized.

Copy link
@diegohaz

diegohaz Nov 12, 2019

Member

I think there may be a styling issue introduced by these changes.

  1. Create a Table block
  2. Change its alignment to full width
  3. Check mobile screen

image

@diegohaz diegohaz mentioned this pull request Nov 12, 2019
5 of 5 tasks complete
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
Those margins were created to accommodate a mover control that was available on hover. Given the mover control is now on-select only, this margin is no longer necessary.

This is a work in progress. When done it needs to not land in 5.3, and it will need a dev-note when ready.
jasmussen added a commit that referenced this pull request Nov 13, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
jasmussen added a commit that referenced this pull request Nov 15, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
jasmussen added a commit that referenced this pull request Nov 18, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
jasmussen added a commit that referenced this pull request Nov 18, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
jasmussen added a commit that referenced this pull request Dec 2, 2019
jasmussen added a commit that referenced this pull request Dec 2, 2019
* Remove horizontal margin from generic appender.

This became unnecessary with #17877.

* Fix similar issue for columns.
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
22 of 23 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.