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

Fix reusable block horizontal padding regression. #21312

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

jasmussen
Copy link
Contributor

https://github.com/WordPress/gutenberg/pull/21099/files#diff-ee2ed3adbe2578628039530c717a9a93R640 introduced a padding attached to the is-root-container. But a reusable block is also a root container.

This PR unsets that.

Before:

Screenshot 2020-04-01 at 09 23 59

After:

Screenshot 2020-04-01 at 09 23 48

@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Apr 1, 2020
@jasmussen jasmussen self-assigned this Apr 1, 2020
@github-actions
Copy link

github-actions bot commented Apr 1, 2020

Size Change: +20 B (0%)

Total Size: 889 kB

Filename Size Change
build/block-library/editor-rtl.css 7.23 kB +11 B (0%)
build/block-library/editor.css 7.23 kB +9 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.4 kB 0 B
build/api-fetch/index.js 3.79 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.54 kB 0 B
build/block-library/style.css 7.55 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 195 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.7 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.23 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.05 kB 0 B
build/edit-navigation/index.js 2.75 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 92.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.1 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.17 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 399 B 0 B
build/editor/editor-styles.css 400 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 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 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 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 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 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.54 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.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.6 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@MichaelArestad
Copy link
Contributor

I'm still seeing top/bottom margin added to the reusable block seen here:

image

It looks like it's caused by one of the wrapping divs preventing neighboring margins from collapsing into each other (I think). I suspect this is the cause:

image

If you uncheck display: flex, the margin collapses.

image

https://github.com/WordPress/gutenberg/pull/21099/files#diff-ee2ed3adbe2578628039530c717a9a93R640 introduced a padding attached to the is-root-container. But a reusable block is also a root container.

This PR unsets that.
@jasmussen jasmussen force-pushed the try/reusable-block-padding-fix branch from fe90811 to abb52be Compare April 8, 2020 06:41
@jasmussen
Copy link
Contributor Author

Nice catch, I'll address that in a separate PR, because this PR is only about the horizontal paddings and therefore relatively "safe", but the fix you suggest needs a little bit more scrutiny.

jasmussen added a commit that referenced this pull request Apr 8, 2020
This is a followup and addition to #21312. It's separate because it's not a regression.
@jasmussen
Copy link
Contributor Author

I created #21472 to fix the vertical margin separately.

Also pushed a small fix to address a navigation mode border shift.

@MichaelArestad
Copy link
Contributor

Thanks @jasmussen. In that case, I think this one is good to go. Nice fix!

@jasmussen
Copy link
Contributor Author

Thanks! Can you approve the PR?

@MichaelArestad MichaelArestad self-requested a review April 8, 2020 18:45
@MichaelArestad
Copy link
Contributor

Done!

@jasmussen jasmussen merged commit 7aa7e36 into master Apr 8, 2020
@jasmussen jasmussen deleted the try/reusable-block-padding-fix branch April 8, 2020 18:46
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 8, 2020
jasmussen added a commit that referenced this pull request Apr 10, 2020
This is a followup and addition to #21312. It's separate because it's not a regression.
jasmussen added a commit that referenced this pull request Apr 10, 2020
This is a followup and addition to #21312. It's separate because it's not a regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants