Patterns: Add wrapper to synced patterns and adopt widest alignment of children #54289
Conversation
aaronrobertshaw
added
[Type] Bug
|
This isn't quite ready for a proper review yet, I've run out of time but will pick it back up Monday. |
|
Size Change: +1.69 kB (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
add/frontend-wrapper-to-patterns
branch
from
7a4f95f
to
bbaf911
Compare
|
Flaky tests detected in e3af74e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6154972354
|
|
Gave it an early look; feels promising :) BeforeCleanShot.2023-09-08.at.20.32.13.mp4AfterCleanShot.2023-09-08.at.20.31.10.mp4 |
aaronrobertshaw
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
aaronrobertshaw
changed the title
|
Most of the layout/alignment quirks have been ironed out I think. So now might be a good time to get a general sanity check on the approach and if there are any obvious issues. I'll sort out the failing pattern tests shortly. |
aaronrobertshaw
force-pushed
the
add/frontend-wrapper-to-patterns
branch
from
03dd0ed
to
49c584d
Compare
There was a problem hiding this comment.
This is testing very nicely for me:
✅ Existing unaltered patterns appear to still render correctly without a wrapper introduced on the site frontend, so a pattern inserted into a Row block still renders correctly, for example
✅ Block markup is updated as expected in the editor when making a change
✅ Alignment controls can be adjusted within the editor within the pattern, and setting to None still allows you to set back to Wide / Full
Just left a couple of tiny questions, but overall the direction seems promising to me, and I didn't run into any issues with it!
I see that the tests for the mobile app had to be updated — will this change cause any issues for older versions of the mobile app accessing newer updated markup?
Edit: as discussed separately this PR still has the issue of layout support classnames being injected into the markup when no wrapper is in use. E.g. for legacy markup in a synced pattern that begins with a heading block, layout classnames get injected into that heading block when they shouldn't be:
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
|
Thanks for the review, testing, and discussion on this one @andrewserong! It has definitely proven a little tricky to iron out all the edge cases.
That's a good question and one that I need to find the answer to. Unfortunately, I went down a rabbit hole on the other issue you noted and have run out of time today. I'll follow-up on this tomorrow.
I believe I have something that addresses this issue. In d598873, I've added a As the One question/concern I have with this latest approach is whether it is acceptable that any |
There was a problem hiding this comment.
I like the pre_render_block filter approach @aaronrobertshaw, that seems the neatest so far while maintaining behaviour in the two different states of the block, without introducing a new API for this one block's requirements.
It's testing nicely for me, with layout block support no longer being applied to legacy markup 👍
One question/concern I have with this latest approach is whether it is acceptable that any render_block filters applied by third parties to this block would now also be skipped for old pattern instances without a wrapper. Perhaps a dev note would suffice?
From my perspective, I think of the core/block as being kind of a special kind of block — it uses the block API, but isn't really a block that's intended for styling over overriding in a typical way, so short-circuiting for this use case feels appropriate to me. A dev note sounds like a good idea.
That said, I don't have a full understanding of what the potential implications might be, so it'd be well worth getting more folks' perspectives on this one.
To me, though, this seems like a very promising direction and looks good to me!
There was a problem hiding this comment.
Just gave this a quick round of testing before looking through the code in depth. A couple of things:
- Is it intended that any change to a template that has an existing pattern in it will cause a wrapper to go around that pattern? This is a bit unexpected.
- I don't think we want to always apply constrained layout to the wrapper. For instance, if I add a Row to a template (not a post) at root level it's full width by default. Say I add a Site Title and a Navigation to it and set the Row to space between, so now I have a block at each end of the row, which is a pretty common header/footer pattern. If I create a pattern from that Row, its contents jump into the middle of the page. Not only is the Row not full width any longer, I am unable to set it to it full width because the pattern wrapper doesn't support alignments for its children. To make things worse, this is not an editor-only problem: the same happens on the front end.
I'm sorry I don't have any ready solutions for these problems. My feeling is that it might be better to avoid making changes to the front end for something that is purely an editor view problem, but I'm also very aware of the extensive discussions around this issue and the fact that so far, no good solution has been found 😬
|
I just remembered I had a thought when looking at Glen's PR: would the skipping serialization route mentioned in that comment be viable at all? We'd probably still need to set layout dynamically depending on the block context but at least we wouldn't have to change anything on the front end |
|
Thanks @andrewserong and @tellthemachines for taking this for a spin.
I'm open to ideas to avoid it if you have any.
Can you provide a video or some example block markup?
We would need to conditionally skip serialization and only on the frontend, which would not match other block supports. Could it be done yes but only in just as hacky a way as anything proposed here. Implementing that whole new API for only this special case doesn't seem ideal.
The desire is to change things on the frontend though as suggested on the original PR. So we are moving towards a consistent display between both front and backends. |
|
@youknowriad if you have the time, I'd love to get your thoughts on this PR or whether we should pivot back to pushing #54170 across the line with some of the lesson's and fixes learnt through exploring this approach. Trying to force a wrapper for only new pattern instances on the frontend while maintaining layout is proving rather difficult. Short of creating a Would you be open to revisiting the earlier approach that does not introduce a wrapper on the frontend, as an interim step? This would provide a smoother pattern creation experience in the editor and buy us time to find a clean solution to introducing the frontend wrapper. I'm conscious of the fact we need to sort out the pattern editing experience for 6.4 and are running out of time. If there is a better option we can do in a few days, I'm open to new ideas. |
|
First, I don't think 6.4 pressure should force us to make any quick decision that we might regret later, this issue for reusable blocks have been around for a long time and one release more or less is not going to change a lot. So let's make sure we make the right decision here regardless of any deadline. 1- #54170 is not that impactful (aside some hacks in the editor), If I'm not wrong it has no impact on the frontend and just "tries" to set the right width of the reusable block wrapper properly. So if we want to merge it, let's make sure that we can revert it without issues/breaking changes... That said, it's very hacky and a bit incomplete. Here are some flows where it fails: 2- Maybe we should create a |
|
Thank you for the feedback @youknowriad 🙇
No arguments here!
That is correct. There were a number of use cases it didn't factor in, that were addressed in this PR. I have created a new PR (#54416) to apply some of these fixes back onto #54170 Among others, these include:
Using the simple filter to add editor-only layout support and the hook to infer alignment and layout in the block's edit component, is about as low impact as we can make it. Nothing is being persisted with this approach, so reverting it would only bring back the current issue where the pattern contents appear to be constrained when they shouldn't be. That visual regression can be mitigated if it's done while applying whatever long-term fix we settle on. After several long discussions with @glendaviesnz, @tellthemachines, and @andrewserong, my preference would be to proceed with #54170 once the fixes from #54416 and any others required have been applied. That improves the editing experience around patterns and buys us time to think through the best long term option, including whether that is a v2 of the
I'll give #54416 some further testing tomorrow, working through the flows identified. Very much appreciate the advice shared, thank you 👍 |
|
Ok sounds good to me. Thanks for considering the alternative, I appreciate it. It does make sense to try the non-impactful approach first. I left a small comment here #54416 (comment) that could improve things too. |
Fixes:
Related:
What?
Why?
It's important to provide a smooth editing experience around patterns. Creating a pattern from wide, full, left, or right-aligned blocks, only to see them move and get squashed into a pattern block wrapper in the editor is unexpected.
How?
core/block)pre_render_blockfilter to short circuit block rendering when an old pattern doesn't have a wrapper so that layout support styles aren't injected into the first child of the pattern incorrectlyrender_block_datafilter to determine the widest alignment of its immediate children while they are processed viado_blocksWP_HTML_Tag_Processoris used to wrap the pattern's entity content with the wrapping element and apply the alignmentNotes
It was raised in #54170 (comment) whether we could move the logic to detect the widest alignment to the point where we create the pattern.
From my explorations, it doesn't look like that or solely relying on updating the pattern's edit component will work. There is a use case where the pattern is created via the block editor and then later edited directly in the
wp_blockpost type editor.An alternative to the filter approach I've taken here might be to save the alignment to the entity's post meta. I opted against this, as we're already parsing the blocks for the pattern via
do_blocksin its render callback, so the filters seemed more straightforward for the moment.Testing Instructions
Backwards Compatibility
Wrapped Patterns
wide,full,left, orrightwide,full,left, orrightalignment.Screenshots or screencast
Screen.Recording.2023-09-08.at.7.16.04.pm.mp4