Try: Remove canvas padding. #22213
Try: Remove canvas padding. #22213
Conversation
jasmussen
added
[Type] Code Quality
|
Size Change: -42 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
I like this a lot but i did try this recently and had issues on mobile where the text was starting from the edge. (probably one of the 20* themes, i'll have to try again to confirm though). |
|
Thank you! If you manage to find the theme you saw this in, I'm happy to think of edgecases and fixes. Edit: Actually I've reproduced, it's when there's no editor style. Let me take a look. |
|
I have this on 2020, it's not broken but there's no padding defined. Do you think it should be the theme responsibility to define the mobile padding? |
youknowriad
added
the
[Feature] Themes
|
I think it is, yes, but I also think that we have to handle it for the vanilla styles (i.e. no editor style). I have an idea that I'll try out and then we can let some opinions trickle in. |
|
So, to expand a bit — ultimately I think the goal is for themes to be able to load the same, or as close to as possible, CSS file into the editor as they load into the theme. Every theme handles mobile like this — some add padding to the body, some apply something like That was actually my idea to try — add a calc-based max-width and then override at the mobile breakpoint. However that adds specificity that we don't want to add. So I have two ideas for moving forward:
Option 2 would benefit in other ways too, as we'd be able to move a couple of our baseline styles over in that stylesheet, so that themes that do have editor styles don't have to deal with them. What do you think? |
To be fair, that was the initial idea of the "editor-styles.scss" stylesheet. it seems that it changed at some point and that it was always loaded in the end (I suspect during the 5.0 merge) but I missed when this happened. |
|
Nice. Should we revisit that? |
|
@jasmussen maybe but I'm afraid about the theme impact, these days we can't change anything without breaking changes :( |
|
Yep, I feel you! I think it's something we should do at some point — but I'm happy to go the conservative route for now. Perhaps if/when we have a combination of iframe and lighter DOM changes, we can bundle together a "careful with the upgrade" package, tear the bandaid off. |
|
Thinking how adding the "no-editor-styles" stylesheet would solve the issue for 2020 and similar themes though? It seems like it wouldn't? |
|
Well that's the thing, 2020 assumes the padding is there and does nothing, but it should, because it does go for the frontend. There's an ugly transition period for some themes, sure. But there will be for lighter dom block changes also, there were for margin changes in the past. I'm not excited for such changes to happen, but I also think it's important to look at what the "right" solution is, even if that's not something we'll get to immediately. |
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
d04125f to
185412b
Compare
So, I just tested, it appears that the contents of So that's the stylesheet that I was asking for, and I've added 10px clearace to the body there in 185412b. What do you think? |
|
If we don't compensate fullwide blocks for the newly added 10px padding, we get this:
However I re-added that padding, and we now get this:
This makes it a much "safer" pull request. There's still that helper padding all around. But it's smaller (10px), it doesn't add specificity by growing at the breakpoint, and it's easy for themes to override. |
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
087806f to
87c97b2
Compare
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
87c97b2 to
01b2cc4
Compare
| // These paddings and margins are removed from the BlockPreview component's style | ||
| // Any change need to be reflected there. | ||
| // The editor-styles.scss file adds 10px padding all around the canvas, to ensure blocks aren't edge to edge on mobile. | ||
| // For full-wide blocks, we compensate for these 10px. | ||
| .block-editor-block-list__layout.is-root-container { |
There was a problem hiding this comment.
Should we move these padding resets to the same file where the padding is added?
There was a problem hiding this comment.
As of 44e945f that would move it to editor-styles.scss. Happy to move it there if you feel it's appropriate? I personally like it because it keeps it together, but it feels... wrong?
There was a problem hiding this comment.
I wanted to be able to do this:
body {
font-family: $editor-font;
font-size: $editor-font-size;
line-height: $editor-line-height;
color: $dark-gray-primary;
padding: 10px;
// For full-wide blocks, we compensate for these 10px.
> .wp-block[data-align="full"] {
margin-left: -10px;
margin-right: -10px;
}
}
but body maps to editor-styles-wrapper, and block-editor__typewriter is the immediate child of that. So we have to target .is-root-container, which we can, but it's not quite as nice. I.e. this works:
body {
font-family: $editor-font;
font-size: $editor-font-size;
line-height: $editor-line-height;
color: $dark-gray-primary;
padding: 10px;
}
// For full-wide blocks, we compensate for these 10px.
.is-root-container > .wp-block[data-align="full"] {
margin-left: -10px;
margin-right: -10px;
}
There was a problem hiding this comment.
I'd use the full selector .block-editor-block-list__layout.is-root-container but I think that's a good approach personally.
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
c6db591 to
ef1c14f
Compare
|
I can't get the tests to pass on this one. I've tried restarting them multiple times, but they keep failing. I would appreciate any insights into whether the failure might be legitimate or not. It's CSS only so it's a bit confusing. |
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
ef1c14f to
45628aa
Compare
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
7 times, most recently
from
c705201 to
2b7f4a0
Compare
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
2b7f4a0 to
edf32d0
Compare
This PR removes the 14px on mobile, 36px on desktop breakpoints padding that is applied to the root container. In the previous iteration of the editor, this padding was necessary to: - on mobile, show the 14px block border that sat 14px outside of the selected block. - on desktop breakpoints, show both the 14px block padding, and make room for the side UI (movers) Because both of those elements are no longer present, we can remove this padding.
jasmussen
force-pushed
the
try/remove-left-right-canvas-padding
branch
from
edf32d0 to
4370b73
Compare
| @@ -21,6 +21,9 @@ describe( 'TypeWriter', () => { | |||
| // Create first block. | |||
| await page.keyboard.press( 'Enter' ); | |||
|
|
|||
| // Create second block. | |||
There was a problem hiding this comment.
@ellatrix Not urgent, but when you have a moment I'd love your take on why this specific commit makes the tests pass.
For context, this PR changes the padding of the editing canvas to be smaller. This leaves more room for themes that don't add this padding themselves, i.e. 2020 which is being used for these tests.
The tests failing were
TypeWriter › should maintain caret position
TypeWriter › should maintain caret position after leaving last editable
I tested both of those using npm run test-e2e packages/e2e-tests/specs/edi tor/various/typewriter.test.js -- --puppeteer-interactive, passing on master but failing on this branch prior to this commit.
My guess is that with the padding as it was, there was enough room on the canvas for the blocks to fit without the typewriter effect setting in. This would explain why simply adding some enter-key presses fixed the test for me.
But it does suggest that this particular test is very sensitive to things style properties like padding, margin, line-height and font size. For example if TwentyTwenty updates the editor style to add this padding back, or to change a block margin, this test will likely fail again. Is there a way to make it more resilient than just adding more blocks? Or conversely, is simply adding more blocks an okay way to fix this test?
|
Updated the tests, and merging. |
This carries over a change that was applied to the Post Editor in #22213. See there for more context.
For the Site Editor, we want to allow post content to be inserted at full width (e.g. Cover Blocks). This is currently impossible (see Automattic/wp-calypso#43640) due to two independent problems. This PR fixes them. 1. [Unlike the Post Editor](https://github.com/WordPress/WordPress/blob/f18870ae4e1cb3cd81136424e43c99d705aa2912/wp-admin/edit-form-blocks.php#L286), we're currently not setting `alignWide` in the Site Editor's settings, which becomes a problem here: https://github.com/WordPress/gutenberg/blob/9cd07a92d463377767f56c2b3985badbd363d98f/packages/block-editor/src/hooks/align.js#L173-L196. 2. In order to specifically make the Post Content block full-width, we need to add `align` to its `supports` attribute. (It's still conceivable to use this block at other widths, e.g. in a setting with a sidebar.) Finally, there remained a 10px padding even after applying these changes. This was fixed by removing block list padding from the Site Editor (a change that had previously been applied to the Post Editor in #22213).
This PR removes the 14px on mobile, 36px on desktop breakpoints padding that is applied to the root container.
In the previous iteration of the editor, this padding was necessary to:
Because both of those elements are no longer present, we can remove this padding. This benefits themes that try to style the editor as best possible to match the frontend.
These screenshots show a theme that loads an editor style that goes almost edge to edge — Hello World should sit 10px from the edge:
Same on mobile:
Full-wide blocks compensated for this, I have removed that compensation, and it works as intended:
Those 10px are intended from the theme, and compare with how that theme looks in master:
It still works fine in the editor when no editor style is loaded:
This PR does not appear to have negative effects in TwentyTwenty:
There's an issue in TwentyNineteen:
There's also an issue with TwentyNineteen in master, but as you can see from the scroll distance, it's been made slightly worse with this PR:
I think it's an important code quality improvement to make, so worth fixing in TwentyNineteen, which probably double-compensates for margins that we are removing here.