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

Cover block: Update the box-sizing attribute #25115

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Sep 7, 2020

Description

I think we should add box-sizing: border-box; to the cover block. In themes with a fixed post width, the combination of a cover block with 100% width and 16px padding means that the cover block extends outside the bounds of the post content:

Screenshot 2020-09-07 at 13 34 51

Adding box-sizing: border-box; tells that block to count the padding as part of the 100% width, and prevents it bleeding out:

Screenshot 2020-09-07 at 13 36 12

In the other themes I tested this doesn't change anything.

How has this been tested?

Tested with TwentyTwenty and Seedlet thems

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@scruffian scruffian added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Sep 7, 2020
@scruffian scruffian self-assigned this Sep 7, 2020
@github-actions
Copy link

github-actions bot commented Sep 7, 2020

Size Change: +21 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-library/style-rtl.css 7.61 kB +9 B (0%)
build/block-library/style.css 7.6 kB +12 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.35 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.53 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/editor-rtl.css 8.6 kB 0 B
build/block-library/editor.css 8.6 kB 0 B
build/block-library/index.js 135 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.78 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 167 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 20.3 kB 0 B
build/edit-site/style-rtl.css 3.51 kB 0 B
build/edit-site/style.css 3.51 kB 0 B
build/edit-widgets/index.js 17.5 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.5 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 621 B 0 B
build/i18n/index.js 3.55 kB 0 B
build/is-shallow-equal/index.js 709 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.61 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

@jasmussen
Copy link
Contributor

I'm a box-sizing: border-box; fan. To the point that I think it should be the default behavior on the web.

However given that this block has shipped for a while without it, and is relatively well used, what kind of testing can we do to ensure this doesn't regress? And, perhaps a larger question, if the motivation for adding this is the padding behavior which was recently added to Cover, but is on its way to Group, should we add this property to more blocks? Which ones?

@kjellr kjellr added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label Sep 10, 2020
@scruffian
Copy link
Contributor Author

I tested this on Twenty Twenty, Twenty Nineteen, Seedlet and Twenty Ten. These lines of CSS didn't make any difference to any of the newer themes, but on Twenty Ten, these lines fixed the display of the block which otherwise was bleeding into the sidebar.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Yep, pairing the box model with the padding feature makes for a good predictable heuristic. This needs a good testing in a few themes, but it should work great. Thanks!

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Tested with a few themes, didn't see any issues

@scruffian
Copy link
Contributor Author

Any idea why the tests are failing?

@jasmussen
Copy link
Contributor

That specific test keeps failing occasionally. I restarted it.

@scruffian scruffian force-pushed the update/cover-border-sizing branch from 1ea0a9b to b801d84 Compare September 24, 2020 12:43
@scruffian scruffian merged commit b0f6704 into master Sep 24, 2020
@scruffian scruffian deleted the update/cover-border-sizing branch September 24, 2020 13:17
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [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.

4 participants