Edit Site: Allow wide alignment #23488
Conversation
|
Size Change: -372 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ockham
requested review from
ajitbohra,
ellatrix,
Soean,
spacedmonkey,
talldan,
TimothyBJacobs and
youknowriad
as code owners
|
cc/ @jeyip (who I can't seem to set as a reviewer 😕 ) |
There was a problem hiding this comment.
I followed the testing instructions. I cleaned the environment, activated 2020-blocks, and went through these steps. I am getting the post content block at full-width (minus the 10px padding). While the cover block is also set to full-width, it is still restricted to the thin width as before:
(crud, github isn't letting me upload right now 😭 )
Also, If I activate 2019-blocks and visit the site editor.. everything is super wide and flowing off the visible page.
| @@ -241,7 +241,7 @@ const BlockComponent = forwardRef( | |||
| // Overrideable props. | |||
| aria-label={ blockLabel } | |||
| role="group" | |||
| { ...omit( wrapperProps, [ 'data-align' ] ) } | |||
There was a problem hiding this comment.
This is like this because the attribute is now applied to a wrapper. This change might be causing the breakage @Addison-Stavlo noticed.
There was a problem hiding this comment.
You mean the <div className="wp-block" data-align="..." /> wrapper? But as of #23089, that's added by block-list/block.js rather than block-wrapper.js. And AFAICS, block-wrapper omitting that prop when instantiating the block component is why that wrapper wasn't added. Or am I missing something?
There was a problem hiding this comment.
gutenberg/packages/block-editor/src/components/block-list/block.js
Lines 160 to 169 in 332397e
I see it on a different div there.
There was a problem hiding this comment.
But that's the one I'm talking about:
You mean the
<div className="wp-block" data-align="..." />wrapper?
Try this, on master, and then on this branch:
diff --git a/packages/block-editor/src/components/block-list/block.js b/packages/block-editor/src/components/block-list/block.js
index 93ae691015..aca5b8bb8f 100644
--- a/packages/block-editor/src/components/block-list/block.js
+++ b/packages/block-editor/src/components/block-list/block.js
@@ -95,6 +95,10 @@ function BlockListBlock( {
);
const isUnregisteredBlock = name === getUnregisteredTypeHandlerName();
+ if ( name === 'core/cover' ) {
+ console.log( { wrapperProps } );
+ }
+
// Determine whether the block has props to apply to the wrapper.
if ( blockType.getEditWrapperProps ) {
wrapperProps = {master:
This branch:
There was a problem hiding this comment.
The block wrapper is a child in this file. How would omitting the prop there make it not show here?
gutenberg/packages/block-editor/src/components/block-list/block.js
Lines 207 to 212 in 332397e
blockEdit is already wrapped at that point.
There was a problem hiding this comment.
Right. I was getting my wires crossed here with regard to which component uses which (and I could've sworn I re-built Gutenberg with and without the change to verify, but apparently not 🤷♂️ )
Anyway, I've reverted the relevant commit, and Twenty Twenty Blocks still looks as before reverting 👍 However, Twenty Nineteen Blocks also still looks garbled 😕
There was a problem hiding this comment.
Sounds like a different issue then?
There was a problem hiding this comment.
|
There's a lot of thoughts to rewrite alignment (#20650). It would be great if this could wait after Beta 1 or everything added is experimental. |
ockham
force-pushed
the
fix/edit-site-full-width
branch
from
cba1766 to
186aa2e
Compare
Thanks for the pointer! Wow, that's a long discussion 😅 This PR currently consists of 3 changes, 2 of which I think are quite limited in scope to the Site Editor (which is experimental anyway). The lack of wide alignment of post content is quite the blocker for site editing, so TBH I'd rather not wait on the outcome of #20650. I think the risk in these changes is fairly low -- curious to hear your thoughts? |
This reverts commit fd2b7dd.
This is coming from here: In the Post Editor, it's compensated by this selector: gutenberg/packages/editor/src/editor-styles.scss Lines 19 to 23 in 6225038 |
In the Site Editor, it's set here: I've noticed that the Post Editor padding/margin was changed very recently: #22213. We might need to do something similar to the Site Editor. |
This carries over a change that was applied to the Post Editor in #22213. See there for more context.
|
Simple removal of the relevant lines seems to fix the 10px padding problem: 6ae4c63 🎉 |
|
Thanks a lot @jeyip! ❤️ @epiqueras It seems like I need your approval in order to merge the PR 🙏 |
Description
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
32 independent problems. This PR fixes all of them.To provide some context for those issues -- the alignment handling is somewhat complex, and happens mostly at
BlockListBlocklevel, using thewithDataAlignHOC.alignWidein the Site Editor's settings, which becomes a problem here:gutenberg/packages/block-editor/src/hooks/align.js
Lines 173 to 196 in 9cd07a9
2. InReverted, see this discussion.block-list/block-wrapper.js, we're omitting thedata-alignprop from wrapper props, before passing them on to the actual element. I believe that this is an oversight from a refactor PR (Block: move align wrapper out of Block element #23089) that moved some logic from the block wrapper intoBlockListBlock. cc/ @ellatrixalignto itssupportsattribute. (It's still conceivable to use this block at other widths, e.g. in a setting with a sidebar.)Fixes Automattic/wp-calypso#43640.
How has this been tested?
wp_templateCPTs (neither published norauto-drafts) from previous Site Editor runs. (It's best to start with a fresh install, i.e.npx wp-env clean all && npx wp-env start. Careful, this will wipe your WordPress install's data!)/wp-admin/admin.php?page=gutenberg-experimentsmust not be ticked. (Make sure the "Full Site Editing" checkbox is ticked -- since it also gets reset after a wipe.)theme-experimentsrepo (see thewp-envREADME).(There's currently still a 10px padding which I'll address separately.)Fixed, see 6ae4c63.Screenshots
Before
After
Types of changes
Bugfix
Follow-up PRs
There's still a 10px padding that needs to be removed.Fixed, see 6ae4c63.theme_supports) rather than via thesettingsvar? Edit: Exploring this in Data: Use getThemeSupports() instead of getSettings() #23566.Checklist: