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 compensating nested blocks for block padding #6408

Merged
merged 3 commits into from
Jun 11, 2018

Conversation

jasmussen
Copy link
Contributor

As we make more and more blocks support nested blocks, we need to think about a way for nested blocks to compensate for their block padding. That's the 14px that surrounds the block itself, and on which the 1px selected-block border is painted.

If we don't, any block that goes from non-nested blocks to nested blocks will suddenly have an extra 14px amount of padding inside.

This PR is an experiment to fix that, and adds compensation before and after any nested context. What's missing here is a fix for collapsing margins — otherwise the negative top and bottom margins will apply to the parent, not the nesting context. The way to fix this is to apply a padding to any context in which childrens margins should not collapse into the parent. This PR adds that to the blockquote block itself, but if we think the general approach in this PR has merit, then we should find a way to make this more generic. For example a block that has nested children, if it had a has-children classname, then we could simply add the padding to that.

Before:

screen shot 2018-04-25 at 13 07 36

After:

screen shot 2018-04-25 at 13 01 25

@jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Apr 25, 2018
@jasmussen jasmussen self-assigned this Apr 25, 2018
@jasmussen jasmussen requested a review from a team April 25, 2018 11:07
@@ -1,6 +1,10 @@
.wp-block-quote {
margin: 0;

// compensate nested blocks for collapsing margins
padding-top: 1px;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to apply those styles to all blocks that use nesting? It might not scale well if we will have to do the same trick for all existing containers like Columns or Pullquote, soon Cover Image or List.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, mentioned in the PR:

This PR adds that to the blockquote block itself, but if we think the general approach in this PR has merit, then we should find a way to make this more generic. For example a block that has nested children, if it had a has-children classname, then we could simply add the padding to that.

So, if we think this approach has merit, what do you think of applying a has-children class to the parent block container? If we add that generic class, I can also make this code generic.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this could be done with hooks in a similar way we add data-align to the block wrappers. Not sure if another class name or data attribute is better, but we could use such general solution 👍

@gziolo
Copy link
Member

gziolo commented Apr 26, 2018

This is how it looks at the moment:

screen shot 2018-04-26 at 10 58 15

screen shot 2018-04-26 at 10 58 57

screen shot 2018-04-26 at 10 59 05

This left-side border is now positioned too low.

@jasmussen
Copy link
Contributor Author

I'll take a look.

How did you get the citation to show up?

@gziolo
Copy link
Member

gziolo commented Apr 26, 2018

It also introduces regression for Columns:
screen shot 2018-04-26 at 11 03 00

How did you get the citation to show up?

I clicked below a few times until it showed up 😄

@jasmussen
Copy link
Contributor Author

Thanks for all the reviews. Do you think you can help me with the data-attribute or class? Pretty please? :D

Not urgent — I won't have time to work on this branch today, and I'm AFK tomorrow.

@gziolo
Copy link
Member

gziolo commented Apr 26, 2018

Do you think you can help me with the data-attribute or class? Pretty please?

Sure thing :)

@jasmussen
Copy link
Contributor Author

Citation issue is tracked in #6404

@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label May 16, 2018
@jasmussen
Copy link
Contributor Author

Blocked on try/alternate-hover-approach.

@jasmussen
Copy link
Contributor Author

I think this is ready for a review again. It has been rebased, and now works with the new outlines. Since the quote is not nested anymore, this is simply a move to make the nested block styles generic.

A future patch will look at enabling collapsing margins, but for now that's not part of this PR.

@jasmussen jasmussen removed the [Status] In Progress Tracking issues with work in progress label Jun 4, 2018
@jasmussen jasmussen requested a review from a team June 4, 2018 06:46
@jasmussen jasmussen added this to the 3.1 milestone Jun 7, 2018
jasmussen and others added 3 commits June 11, 2018 08:37
As we make more and more blocks support nested blocks, we need to think about a way for nested blocks to compensate for their block padding. That's the 14px that surrounds the block itself, and on which the 1px selected-block border is painted.

If we don't, any block that goes from non-nested blocks to nested blocks will suddenly have an extra 14px amount of padding inside.

This PR is an experiment to fix that, and adds compensation before and after any nested context. What's missing here is a fix for collapsing margins — otherwise the negative top and bottom margins will apply to the _parent_, not the nesting context. The way to fix this is to apply a padding to any context in which childrens margins should not collapse into the parent. This PR adds that to the blockquote block itself, but if we think the general approach in this PR has merit, then we should find a way to make this more generic. For example a block that has nested children, if it had a `has-children` classname, then we could simply add the padding to that.
@jasmussen
Copy link
Contributor Author

@gziolo rebased, are your questions addressed? Thanks.

@gziolo
Copy link
Member

gziolo commented Jun 11, 2018

All good 👍

@jasmussen
Copy link
Contributor Author

Yay, thank you!

@jasmussen jasmussen merged commit ebb289f into master Jun 11, 2018
@jasmussen jasmussen deleted the try/nested-blocks-margins branch June 11, 2018 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants