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: Fix nesting container margin smarts. #25527

Merged
merged 1 commit into from
Sep 22, 2020
Merged

Conversation

jasmussen
Copy link
Contributor

In #24966 (comment) padding support was added to the group block. As part of that, the first and last child of child blocks had their top and bottom margins zeroed out. The intention was noble: to make the padding act more predictably in a world where margin collapsing is a nightmare hellride to understand.

However it had a side effect — the group has an appender block at the end, which grabs that bottom margin rule, unless an adjacent block is selected:

appender

Screenshot 2020-09-22 at 09 36 51

This PR removes that rule, which although the intent was noble, it has consequences. The rules of margin collapsing really are weird and unpredictable, but I worry that trying to outsmart them we'll eventually create so many edgecases that it just ends up adding complexity. Here's after:

Screenshot 2020-09-22 at 09 44 32

Yes I know, my in development theme has crazy tall margins:

Screenshot 2020-09-22 at 09 40 06

@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Sep 22, 2020
@jasmussen jasmussen self-assigned this Sep 22, 2020
@github-actions
Copy link

Size Change: -52 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-library/editor-rtl.css 8.56 kB -26 B (0%)
build/block-library/editor.css 8.56 kB -26 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.34 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 8.41 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/index.js 135 kB 0 B
build/block-library/style-rtl.css 7.6 kB 0 B
build/block-library/style.css 7.59 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.77 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 202 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 1.27 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.4 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.24 kB 0 B
build/edit-post/style.css 6.23 kB 0 B
build/edit-site/index.js 19.6 kB 0 B
build/edit-site/style-rtl.css 3.3 kB 0 B
build/edit-site/style.css 3.3 kB 0 B
build/edit-widgets/index.js 17.6 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.8 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.8 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 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 13.7 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad
Copy link
Contributor

Looks like someone beat me to it and the fix in #25518 also fixes this issue.

@jasmussen
Copy link
Contributor Author

Looks like someone beat me to it and the fix in #25518 also fixes this issue.

Well, sort of:

sort of

The appender still gets shown when you select the block, as it should. Which means it becomes the last-child when the block is selected. In comparison, here's this branch:

compare

The change in this branch, even if it isn't as smart when it comes to paddings, still feels like the more resilient and predictable behavior for me.

@youknowriad
Copy link
Contributor

I disagree, I think we should keep the style unless we remove all the margins, otherwise the behavior is inconsistent.
I opened an alternative fix in #25530

for example, if you test this PR in 2021 theme, you'll notice the margin jump is still there.

@youknowriad
Copy link
Contributor

Testing more, I'm not reproducing the issue in 2021 anymore, let me think this through.

@youknowriad
Copy link
Contributor

So I think ideally we find a selector that says "last block"/"first block" excluding any extra divs. I'm not able to do that right now for some reason.

The reason I think this is important because these margins are going to interfere with paddings applied to the Group block. Say you define a custom small padding to your group block, the padding on the bottom/top won't be correct because the margins will apply there.

I'm ok with this though until we find a better way.

@jasmussen
Copy link
Contributor Author

You're a man of reason, Riad. I agree with you on a human level even if occasionally we have separate approaches on the superficial level. Thanks for all your in depth thoughts on this.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants