Patterns: Set width of synced pattern editor block wrapper to max width of widest child #54170
Conversation
glendaviesnz
added
[Type] Bug
|
Size Change: +242 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
What an interesting problem!
From testing, this looks like it largely resolves the issue in the post editor, while also fixing up block spacing between blocks — provided that it's expected that synced blocks will be rendered somewhere within a constrained layout.
| Before | After |
|---|---|
 |
 |
Unfortunately opting in to the layout support directly does appear to result in unintended classnames being rendered in the PHP output (I've left a separate comment about that).
I'm wondering if a possible path forward could be to cheat it, so that instead of opting the block in to the layout support, could you call useLayoutClasses within edit.js and then manually add the classnames there in the edit view? That could also add a bit of flexibility if it's important to use the layout of the parent to determine which layout type to use, too... but I'm not sure if that's a requirement for this fix.
| }, | ||
| "layout": { | ||
| "type": "array", | ||
| "default": { | ||
| "type": "constrained" | ||
| } |
There was a problem hiding this comment.
Unfortunately it looks like adding layout support here results in the PHP code for layout being executed on the rendered markup, which is usually what's expected for blocks that opt-in to layout support. For this one, because the wrapper only exists in the editor, the PHP version of the layout support winds up applying the layout classnames to the first block within the rendered markup. For example, if a synced block contains an Image block as the first block, it winds up getting is-layout-flow and wp-block-block-is-layout-flow applied to the markup:
@glendaviesnz can I check if this is different than the approach I took previously in #31082? I know that approach was abandoned as unworkable but perhaps there's been progress since then I'm unaware of? |
|
Thanks @getdave and @Mamaduka, appreciate the ping, this is dangerous territory I know 😄. This is very much borrowing from #31082, but with the addition of adding hidden layout support to the reusable block wrapper so that wide/full layouts can still be applied by the children. Still very experimental at this stage, and very cognisant of the difficulty of solving this issue in a backwards compat way. |
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
Thanks @andrewserong, we are already manually applying the classname in the editor anyway, but the children get the align options based on the layout support of the parent I think rather than the parents classnames. Adding skip serialization support to layout seems to fix the issue of the stray classnames though. |
|
Flaky tests detected in 98707fb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6169704519
|
Ah, of course that makes sense. One potential issue with introducing Since the objective is for the block to have the appearance of opting-in to the layout support without it really opting-in, could an alternative be to somehow include the layout support in only the JS registration of the block, but not the server-side one? I.e. instead of adding in the support in |
Good point, I have switched to just explicitly excluding |
| @@ -45,8 +55,38 @@ export default function ReusableBlockEdit( { attributes: { ref } } ) { | |||
| ref | |||
| ); | |||
|
|
|||
| const alignments = [ 'wide', 'full' ]; | |||
|
|
|||
| useEffect( () => { | |||
There was a problem hiding this comment.
I think the presence of the setAttributes calls in this useEffect results in a post's Update button not being disabled when reloaded in the editor, since the editor believes that something has changed. Here's a quick gif of reloading the editor:
There was a problem hiding this comment.
yeh, I don't like setAttribs in useEffects, but was struggling to see a different way to handle this - but will take another look.
There was a problem hiding this comment.
trying to handle this outside a useEffect causes trying to render while something else is still rendering errors. I have added __unstableMarkNextChangeAsNotPersistent for now which prevents the editor from seeing the post as dirty after the updates. Is sort of nice having this anyway as makes it clear that these attribute changes are not persisted.
Let me now if you can see a way of running this on changes to blocks without using a useEffect.
There was a problem hiding this comment.
Can we move this logic into the reusable block creation action instead ?
There was a problem hiding this comment.
Can we move this logic into the reusable block creation action instead ?
The source pattern can be edited via the wp_block post type editor as well. We'd need to make sure the pattern instance reflected any changes in child alignment there. Same for the frontend, once we add the wrapper.
There's a more detailed comment on this in the PR description for #54289 which attempts to add the frontend wrapper in a BC way.
| @@ -10,6 +10,12 @@ | |||
| "attributes": { | |||
| "ref": { | |||
| "type": "number" | |||
| }, | |||
| "layout": { | |||
There was a problem hiding this comment.
If I'm not mistaken, I think you might not need to explicitly add the layout attribute in order to set a default? Within the layout object in supports, you can set a default object. For example, here's the one from the Columns block:
gutenberg/packages/block-library/src/columns/block.json
Lines 48 to 56 in ad92865
There was a problem hiding this comment.
That seems to work - and saves having a documented attrib for the block that shouldn't actually be used. Thanks.
|
I found my notes for this issue. I was also planning to work on it, but I got carried away with other shiny things. :) I will share them, just in case they're helpful to anyone. Notes
|
|
Thanks for the extra context @Mamaduka
This may just be a temporary approach to fix the broken feel of the block for wide and full align - and trying to do it in a way that does not affect the serialized markup at all so that the approach is easily changed down the line if we do move to a wrapper. Still very draft and a way of exploring all the possible alternatives for a possible fix for 6.4. Will keep you in the loop if we get past the exploration phase. |
| 'blocks.registerBlockType', | ||
| 'core/synced-patterns/add-editor-only-layout-support-for-patterns', | ||
| addEditorOnlyLayoutSupportForPatterns | ||
| ); |
There was a problem hiding this comment.
Why is this done in a filter instead of just updating the block.json?
There was a problem hiding this comment.
Because we don't want the layout supports serialized in the frontend by the server-side rendering. Without the wrapper on the frontend it adds the layout classes to the first child block instead which causes issues.
We did look at adding __experimentalSkipSerialization to layouts, but it was a bit quirky only applying this on the server end as all other uses of __experimentalSkipSerialization apply to both js and php sides.
We also explored adding a new __experimentalSkipServerSerialization, but this leaves us with yet another API that needs to be supported, just for what is currently a single edge case.
In the end we decided that given this is a background/hidden use of the support to get around the layout quirk in the editor only, that the filter was the best approach, but open to other suggestions.
There was a problem hiding this comment.
BTW, we are seeing this as a quick fix for the most obvious/immediate issue with the wide/full layouts. It is not perfect but the main advantage of this approach is that it does not serialize any changes to the block content so makes it easier to then move to the wider/more long-term fixes covering more layout flexibility mentioned in #8288
There was a problem hiding this comment.
Because we don't want the layout supports serialized in the frontend by the server-side rendering. Without the wrapper on the frontend it adds the layout classes to the first child block instead which causes issues.
Can you clarify this, why don't we want that? There something I might be missing?
There was a problem hiding this comment.
Oh I guess because the reusable blocks have no wrapper in the frontend 🤔 I feel we should solve that first as otherwise it's going to be hacks on top of hacks. Maybe we can create the wrapper for reusable blocks v2 basically and leave the existing ones untouched.
There was a problem hiding this comment.
Adding a wrapper also solves the issue of reusable blocks showing up differently between frontend and backend.
There was a problem hiding this comment.
ok, will try and modify the approach to also add the wrapper on the frontend for new blocks only.
There was a problem hiding this comment.
@Mamaduka I probably won't get back to this until next week as I have some pattern category stuff to wrap up first, so feel free to take a look in the meantime if you get your other shiny things all polished up 😃
There was a problem hiding this comment.
I also need to stabilize the Table of Contents block before WP 6.4, so I'm not sure how much time I'll have outside of that. But I will give it a try if I find extra time :)
There was a problem hiding this comment.
I'm probably missing some context here, but shouldn't we be able to get the correct alignments by adding layout to the wrapper and skipping serialization on both server and editor? The alignment code only looks at the layout type, which should be present even if serialization is skipped, right?
| blockSettings.supports = {}; | ||
| } | ||
| blockSettings.supports.layout = { | ||
| allowSizingOnChildren: true, |
There was a problem hiding this comment.
allowSizingOnChildren is only relevant for flex type layouts so it won't apply here.
|
After recent discussions on how best to solve the synced patterns alignment problem, the general consensus was to try wrapping synced patterns on the frontend to match the editor. I have a draft PR that attempts to do this in a backwards-compatible manner - #54289. It isn't quite ready for review as I've run out of time today. I still need to fully get my head around the layout support for this use case. As it stands, if you remove the last wide or full alignment from an immediate child of the pattern, the pattern has a layout config that now prevents wide or full alignments being reapplied. |
… than using skipSerialization to avoid future confusion
…into layout support
aaronrobertshaw
force-pushed
the
fix/synced-pattern-width-issue
branch
from
1373042
to
98707fb
Compare
|
Thanks for all the efforts in exploring solutions here @glendaviesnz 👍 I'll close this as #54416 has been merged fixing the original issue in a slightly different way. |
What?
Checks the align setting of child blocks of a synced pattern and sets the editor block wrapper to the same so that the width is respected in the editor. Also adds layout support of type
constrainedto block wrapper if wide or full align are found to ensure that alignments are correctly applied.Why?
Because synced blocks have a wrapper in the editor, but not the front end, the full and max-width settings are not respected.
How?
Find the widest align option of the child blocks and apply this same align class to the parent wrapper.
Testing Instructions
Screenshots or screencast
Before:
pattern-width-before.mp4
After:
pattern-wide-after.mp4