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.

Sign up for GitHub

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

Group: Zero out the intrinsic margin set to the Group block #22209

Merged
merged 2 commits into from
May 11, 2020
Merged

Group: Zero out the intrinsic margin set to the Group block #22209

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2287669
Group: Zero out the intrinsic margin set to every block in the editor.
jasmussen May 8, 2020
b5e56cc
Address feedback.
jasmussen May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/block-library/src/group/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
/**
* Group: All Alignment Settings
*/
.wp-block[data-type="core/group"] {

.wp-block-group {
// Zero out the baseline margin that is set for every block in the editor.
margin-top: 0;
margin-bottom: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this apply to the first and last block inside groups instead of the group itself? I'm having hard time understanding what's specific about the group block here compared to say "cover".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I think Cover should also zero out margins. #22208 is kind of the nuclear option, this "per-block zeroing out" is the more conservative alternative.

The thing is, both cover and group are divs so they should be "born without margins" just like their HTML counterparts. This wasn't always feasible for the editor, but as we get closer to FSE and such, we really are seeing some opportunities play out here.

Specifically, you might see a Layout Grid block inside a Group block, and the Layout Grid uses flex so its margins can't collapse, so those margins are zeroed out as well. In those cases, it really helps that the group becomes kind of "invisible". When we reset the margin on the block itself, there's nothing to compensate for, and no complicated first/last child rules that can break in various ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually speaking, I still wonder whether this should apply to first and last inner blocks or the parent one. It's not clear to me what's different between a container block and a non container block in terms of margins when shown at the same level while I can understand that the first and last block of a "Block list" don't have top and bottom margins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the first and last blocks of a block list very probably should have margins in some cases, and not have margins in others. That's why I like to think of this same as html an css. If we add a margin and padding control to virtually every block through global styles, going the basic route will make for predictable behavior, whereas going the first child/last child route might add confusion because of the imperceptible heuristic.

To flip it on its head, what would be the benefit if the first/last child approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we add a margin and padding control to virtually every block through global styles, going the basic route will make for predictable behavior, whereas going the first child/last child route might add confusion because of the imperceptible heuristic.

For me, both approaches will have the same result if a padding control is added since both will override the padding for the group (or its first/last block). I don't think there's any big difference tbh between both approaches, my cartesian mind don't like that different blocks on the same block lists have different margins that's all. (feel free to ignore, it's not that important)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would never ignore someone who's so often right. On weekends, I'm redesigning my website, and finding a need for adjusting margins and paddings on many blocks. I expect this to become very common when FSE efforts mature, and that we will increasingly be looking a a canvas where most blocks are born with no margin at all, which we can then adjust on a per-block basis.

We'll absolutely see how it plays out — but just due to how flexible CSS has been with its approach, I feel the most comfortable mimicking that going forward.


// Ensure not rendering outside the element
// as -1px causes overflow-x scrollbars
Expand Down