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

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

Merged
merged 2 commits into from
May 11, 2020

Conversation

jasmussen
Copy link
Contributor

This PR is a much smaller alternative to #22208. It sets the top and bottom margin of a group block to zero, in the editor only, to unset the margins that are otherwise inherited by the baseline margin that every block in the editor has.

This helps a great deal for when people create block patterns, where you might have several groups in a row, that should not have additional margin above and below.

Important also to remember that the margins of contents inside the group can still "bleed out" of the group, and even collapse with adjacent margins.

It would be nice to not have to unset these margins, and simply not have any margins in the first place. But per the discussions in #22208 removing those is nontrivial.

Editor, before:

before editor

Editor: after, after:

after editor

Frontend before:

before frontend

Frontend, after:

after frontend

2 appenders in a row before:

before

After:

after

This PR is a much smaller alternative to #22208. It sets the top and bottom margin of a group block to zero, in the editor only, to unset the margins that are otherwise inherited by the baseline margin that every block in the editor has.

This helps a great deal for when people create block patterns, where you might have several groups in a row, that should not have additional margin above and below.

Important also to remember that the margins of contents inside the group can still "bleed out" of the group, and even collapse with adjacent margins.

It would be nice to not have to unset these margins, and simply not have any margins in the first place. But per the discussions in #22208 removing those is nontrivial.
@github-actions
Copy link

github-actions bot commented May 8, 2020

Size Change: +9 B (0%)

Total Size: 824 kB

Filename Size Change
build/block-library/editor-rtl.css 7.08 kB +5 B (0%)
build/block-library/editor.css 7.09 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.61 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 10.3 kB 0 B
build/block-editor/style.css 10.3 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.28 kB 0 B
build/block-library/style.css 7.29 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 180 kB 0 B
build/components/style-rtl.css 17 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.4 kB 0 B
build/edit-navigation/style-rtl.css 618 B 0 B
build/edit-navigation/style.css 617 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.2 kB 0 B
build/edit-site/style-rtl.css 5.21 kB 0 B
build/edit-site/style.css 5.21 kB 0 B
build/edit-widgets/index.js 8.37 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.14 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

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.

This makes a lot of sense. It removes the double-margins that were visible in the editor before, and gets things looking much more similar to the way they are in the front end. The group block becomes a lot more invisible, which I think is the way it was intended to be.

Screen Shot 2020-05-08 at 8 28 45 AM

My only followup question is whether we should do something similar for the Columns block? That also appears to have "double" margins — the margins from its child blocks, as well the parent block's margins:

Screen Shot 2020-05-08 at 8 28 26 AM

@kjellr kjellr added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label May 8, 2020
@@ -1,7 +1,11 @@
/**
* Group: All Alignment Settings
*/

.wp-block[data-type="core/group"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not .wp-block-group?

.wp-block[data-type="core/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.

@jasmussen
Copy link
Contributor Author

My only followup question is whether we should do something similar for the Columns block? That also appears to have "double" margins — the margins from its child blocks, as well the parent block's margins:

Yes, I think we should, and to Riad's point above — should we do it also for the Cover block? Any others? Of course at some point we veer close to simply wanting to embrace #22208. But I'm happy to followup with any blocks we agree should follow this lead.

@jasmussen jasmussen merged commit af44574 into master May 11, 2020
@jasmussen jasmussen deleted the try/remove-group-baseline-margin branch May 11, 2020 07:34
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone May 11, 2020
@jasmussen jasmussen changed the title Group: Zero out the intrinsic margin set to every block in the editor. Group: Zero out the intrinsic margin set to the Group block Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block [Feature] Custom Editor Styles Functionality for adding custom editor styles [Feature] Themes Questions or issues with incorporating or styling blocks in a theme.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants