-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Block styles: remove box-sizing rule on Post Editor container to achieve editor/frontend parity #37902
Conversation
…-box in the editor is reflected on the frontend.
Size Change: +1.18 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Hi, can you share the block markup used in the images? |
Thanks for the draft PR! I think this rule has been discussed back and forth a few times, and there are many upsides. There are downsides too, such as the case where you want the padding or border to expand the width of the box, as opposed to squeezing it inwards. @jameskoster noted one example. There's a risk that for those edgecases there isn't a way to toggle the behavior off for a block, without adding new CSS to your theme for that case. Given how many themes add this blanket rule by default and just keep it there, however, I'm personally in favor of this landing, given the more intuitive box model it brings with it. But it's something that could use many more thoughts: @kjellr @MaggieCabrera @richtabor @scruffian @Mamaduka to just ask a few additional folks at random. If we do move forward, we can probably remove the editor from the previous reset, as common.scss should be loaded in the editor already. And if we don't move forward, an alternative would be to remove the box-sizing behavior from the reset, ro at least omit the editor from getting it, so at least the editor and frontend behave identically. |
You bet! Thanks for asking. Example code
<!-- wp:group {"style":{"spacing":{"padding":{"top":"92px","right":"92px","bottom":"92px","left":"92px"},"blockGap":"0px"},"border":{"width":"61px","style":"solid"}},"backgroundColor":"luminous-vivid-orange"} -->
<div class="wp-block-group has-luminous-vivid-orange-background-color has-background" style="border-style:solid;border-width:61px;padding-top:92px;padding-right:92px;padding-bottom:92px;padding-left:92px"><!-- wp:heading -->
<h2 id="group-1">Group 1</h2>
<!-- /wp:heading --></div>
<!-- /wp:group -->
<!-- wp:group {"backgroundColor":"pale-pink"} -->
<div class="wp-block-group has-pale-pink-background-color has-background"><!-- wp:heading -->
<h2 id="group-2">Group 2</h2>
<!-- /wp:heading --></div>
<!-- /wp:group -->
<!-- wp:heading {"backgroundColor":"vivid-purple"} -->
<h2 class="has-vivid-purple-background-color has-background" id="heading">Heading</h2>
<!-- /wp:heading -->
<!-- wp:list {"backgroundColor":"secondary"} -->
<ul class="has-secondary-background-color has-background"><li>A list</li></ul>
<!-- /wp:list -->
<!-- wp:table {"style":{"color":{}},"className":"is-style-stripes"} -->
<figure class="wp-block-table is-style-stripes"><table><tbody><tr><td></td><td>Table</td></tr><tr><td></td><td>Table cell</td></tr></tbody></table><figcaption>Table caption</figcaption></figure>
<!-- /wp:table -->
<!-- wp:columns {"backgroundColor":"pale-cyan-blue"} -->
<div class="wp-block-columns has-pale-cyan-blue-background-color has-background"><!-- wp:column -->
<div class="wp-block-column"><!-- wp:heading -->
<h2 id="column">Column</h2>
<!-- /wp:heading --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->
<!-- wp:pullquote -->
<figure class="wp-block-pullquote"><blockquote><p>Pullquote</p></blockquote></figure>
<!-- /wp:pullquote -->
<!-- wp:cover {"overlayColor":"pale-cyan-blue","isDark":false} -->
<div class="wp-block-cover is-light"><span aria-hidden="true" class="has-pale-cyan-blue-background-color has-background-dim-100 wp-block-cover__gradient-background has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size">Cover block</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->
<!-- wp:heading {"level":3} -->
<h3 id="what-follows-is-a-gallery">What follows is a gallery</h3>
<!-- /wp:heading -->
<!-- wp:gallery {"linkTo":"none"} -->
<figure class="wp-block-gallery has-nested-images columns-default is-cropped"><!-- wp:image {"id":12,"sizeSlug":"large","linkDestination":"none","className":"is-style-default"} -->
<figure class="wp-block-image size-large is-style-default"><img src="http://localhost:4759/wp-content/uploads/2022/01/AdobeStock_172853026.jpg" alt="" class="wp-image-12"/></figure>
<!-- /wp:image -->
<!-- wp:image {"id":11,"sizeSlug":"large","linkDestination":"none","className":"is-style-default"} -->
<figure class="wp-block-image size-large is-style-default"><img src="http://localhost:4759/wp-content/uploads/2022/01/electric-guitar-4064538_960_720-1.jpg" alt="" class="wp-image-11"/></figure>
<!-- /wp:image --></figure>
<!-- /wp:gallery -->
<!-- wp:media-text {"align":"","mediaId":10,"mediaLink":"http://localhost:4759/?attachment_id=10","mediaType":"image"} -->
<div class="wp-block-media-text is-stacked-on-mobile"><figure class="wp-block-media-text__media"><img src="http://localhost:4759/wp-content/uploads/2022/01/Guild-Jetstar-Vintage-White-Electric-Guitar-Front-1.jpg" alt="" class="wp-image-10 size-full"/></figure><div class="wp-block-media-text__content"><!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size">Media block</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text --> |
I like this idea. My preference is to apply the rule to the frontend as I think that's a common reset that many themes would benefit from, but there may be cases where this breaks themes. I agree with @jasmussen, if we don't move froward with this we should remove the reset from the editor so at least we have parity. |
I tested a bunch of core blocks in the site editor too. Without the changes in this PR, the Site Editor reflects the frontend, that is, no blanket box-sizing: I tested on a bunch of core themes (2017++) and the blocks that stand out are headings (including site title), which do not declare any box-sizing internally. I think we're agreed that it should be consistent, the only question is what would be a low-friction approach.
Maybe this is the right approach. We won't know the full effect of forcing box-sizing on the frontend and site editor. At least, if we treat the post editor as the outlier for now and bring it in line, people's sites won't break. Here's what we're doing in the post editor: .edit-post-header,
.edit-post-visual-editor,
.edit-post-text-editor,
.edit-post-sidebar,
.editor-post-publish-panel,
.components-popover,
.components-modal__frame,
.edit-post-editor__inserter-panel {
box-sizing: border-box;
*,
*::before,
*::after {
box-sizing: inherit;
}
} It looks like we could remove |
Just looking at that screenshot it really feels like border and padding behaviors would benefit from the rule being applied. That said I do also share the notion that the most delicate step we can do at this point is to start by removing it since it's not necessarily there on the frontend 👍 👍
We'll want to make sure no in-canvas blocks have come to accidentally rely on this behavior. I don't think this should be the case, but some complex blocks like Navigation or Cover would be worth sanity checking just to see that the nested UI for resizing and otherwise still behaves as intended. Finally there are still block UI elements in the canvas that might need the rules, things like placeholders. In that light, we could potentially target |
…ere is no corresponding rule in the frontend or site editor.
I've flipped this PR to remove box-sizing from the visual editor (post editor)
The Cover Block explicitly sets its own The Navigation Block doesn't define
Placeholders also set their own In the testing I've done it looks like they behave. 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happy to try this simply to add parity between editor and frontend. Whatever errors this parity might uncover in the editor should then at least be consistent with what's on the frontend, which in turn could let us revisit and add the blanket reset for themes on top of this.
I'd encourage more testing if you can get it. Thanks for working on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing I didn't see any regressions. That being said, this is a seemingly small change but which can have significant impact that we can't see right now.
Long-term I agree with @jasmussen that it's worth trying this. If anything breaks, we can fix it on a per-use-case basis instead of adding a blanket rule - which would be better for editor/frontend parity in the future.
This has a +1 from me 👍
Thanks for the 👀 @jasmussen and @aristath 🙏 I'll leave it open for another round of testing before hitting the merge button.
Agree. If bugs do arise, at least we'll know what the blanket rule was "concealing" and it might help us tighten up our rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @ramonjd! I agree with the others, I think this will be a good one to land and it'll hopefully reveal places where there are gaps in individual blocks rather than obfuscating the issue within the post editor.
Interestingly, while testing this out (after creating the post content in Twenty Twenty Two), I noticed that setting a horizontal margin value on a heading block doesn't play nicely with some classic themes (e.g. Twenty Twenty One — the non-blocks version) where the centered view of the blocks on the front end use left/right margins to do so:
Fortunately those classic themes don't opt-in to the margin support it seems (so a user can't really wind up in this situation easily), but separately to this PR, I think it highlights again the challenge dealing with margin support (particularly horizontal margins) on blocks. But that discussion / consideration is of course very much separate to this PR.
LGTM!
Description
For editor blocks, there's a blanket CSS rule for box sizing:border-box based on a reset mixin.
No such blanket rule exists on the frontend or in the Site Editor.
So you might see the following:
Many blocks, for example, Group, Column, Cover, already define their own
box sizing:border-box
rule.As you can see from the above screenshots, Heading and List do not.
This PR proposes to remove the box-sizing rule for elements in the Post Editor (visual editor) to ensure the Post Editor ❤️ Site Editor ❤️ Frontend parity.
This change could affect existing blocks and patterns with the assumption that some users may expect the editor and frontend to differ in these respects.
The arguments for this PR are:
box sizing:border-box
rule.For context, see discussion over at:
#37818 (review)
How has this been tested?
Add patterns, blocks (make sure to liberally add Heading and List blocks) to a new post.
Apply borders, padding and margins where the block supports them.
Test in various core themes.
In all scenarios, check that the
box sizing:border-box
is consistent across the frontend and Site/Post editors.Types of changes
Removing a CSS rule from the post editor
Checklist:
*.native.js
files for terms that need renaming or removal).