Pattern block: Add experimental flag and syncStatus attrib to allow testing of partial syncing #50533
Conversation
|
Size Change: +1.37 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in b4e01bb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4943750595
|
glendaviesnz
requested review from
spacedmonkey,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
There was a problem hiding this comment.
Thanks for continuing to iterate on the pattern block experiments @glendaviesnz 👍
For the most part, this tests pretty well.
I did run into a block invalidation error though when reloading the editor with a saved partially synced pattern. In addition to that, there were some other minor things we could clean up.
- Escaping HTML attributes in the render callback
- PHP linting errors
- Is
inheritedAlignmentaccurate if the pattern's blocks are its children? The awkward wording might lead to some minor confusion.
| }, | ||
| "textdomain": "default", | ||
| "attributes": { | ||
| "slug": { | ||
| "type": "string" | ||
| }, | ||
| "inheritedAlignment": { |
There was a problem hiding this comment.
The naming of this attribute, inheritedAlignment, could be a little awkward as "inheriting" from a pattern's inner or child blocks doesn't make much sense to me.
There was a problem hiding this comment.
It might be worth moving this change into a separate PR, as I'm sure there will be some other thoughts on it. I think the main thing right now is to get a first implementation of the partial syncing behavior, so this isn't something of high urgency, though it is something that needs to be fixed.
Some random thoughts - I wonder if it could be made part of the align block supports. While other blocks might have something like align: [ 'wide', 'full' ], this could possibly be align: 'auto'? When set to that it could avoid showing the UI. Just throwing some ideas out there. It might be good to start some discussions with other contributors who may have thought about this problem.
There was a problem hiding this comment.
As they say 'Insanity is inherited, you get it from your children', so the same could apply here 😄. I have removed it now anyway. As Dan suggests, probably better as a follow-up PR so as not to slow things down if people have different opinions on it.
There was a problem hiding this comment.
Thanks for the updates @glendaviesnz.
I'm now getting a block validation error when I paste in a partially synced pattern using the example in the test instructions: <!-- wp:pattern {"slug":"twentytwentythree/cta", "syncStatus":"partial"} -->
Screen.Recording.2023-05-12.at.1.49.19.pm.mp4
There's also a further tweak we need to make when escaping the classname HTML attribute.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
There was a problem hiding this comment.
The latest changes LGTM 🚢
I no longer receive invalid block errors when adding a pattern or reloading a post with one in it.
For posterity, I still think we should note in this PR's description that the partially synced pattern block will impact the overall alignment of the inner blocks until we follow up on that separately.
Done. Do you think we should remove the layout attribute on this PR before merging given it isn't currently being used? |
I'd say so, the cleaner the history, the easier it will be for others (our future selves included) to follow when and why things were added. |
@aaronrobertshaw I have removed all the layout supports if you want to give it a quick last check before we merge it. |
glendaviesnz
deleted the
update/pattern-block-to-allow-partial-sync-option
branch
|
I've updated the pattern markup in the test instructions. It's important to have the correct closing to the delimiter ( Further background: #50629 (review) |
| $classnames = isset( $attributes['className'] ) ? $attributes['className'] . ' ' . $slug_classname : $slug_classname; | ||
| $wrapper = '<div class="' . esc_attr( $classnames ) . '">%s</div>'; | ||
|
|
||
| if ( isset( $attributes['syncStatus'] ) && 'unsynced' === $attributes['syncStatus'] ) { |
There was a problem hiding this comment.
It doesn't appear to be possible to ever have unsynced as a value:
There was a problem hiding this comment.
The thinking was that if there is no syncStatus, ie. syncStatus===undefined then the pattern is not synced, which is partly a backwards compat approach as currently none of the patterns in the wild are synced and they have no syncStatus attrib. We could add another syncStatus of unsynced but we will then have to account for both that and undefined as an indicator of not synced.
Adding unsynced however may make it clearer to people browsing the block.json that there is an unsynced option and avoid the confusion you experienced. I don't have a strong opinion either way. @talldan, @aaronrobertshaw what do you think?
There was a problem hiding this comment.
Good catch @gziolo 👍
The thinking was that if there is no syncStatus, ie. syncStatus===undefined then the pattern is not synced
That's what the block.json attribute definition indicates. What I think @gziolo is questioning is the index.php code checking for an unsynced value. That appears to be a leftover from the original PR where unsynced was a valid syncStatus value and its default.
There was a problem hiding this comment.
🤦 missed the first code snippet in the original comment! Ignore all of my previous comment, I will get the index.php file sorted.
There was a problem hiding this comment.
| if ( empty( $attributes['slug'] ) ) { | ||
| return ''; | ||
| } | ||
| $slug_classname = str_replace( '/', '-', $attributes['slug'] ); |
There was a problem hiding this comment.
Should the same class get generated in the edit implementation?
There was a problem hiding this comment.
Thanks, yes it should, have added a PR for this.
What?
If the syncStatus attribute of a pattern is set to
partiala pattern block wrapper is added and and templateLock is set tocontentOnlyWhy?
See #50456 for some background discussion.
How?
If pattern has
syncStatus===partialadds a parent div that wraps the content of the pattern. Also currently defaults the innerBlocks templateLock tocontentOnly.This is obviously going to change quite a bit in order to get partial syncing actually working as discussed here, but it at least gives us a starting point of having Pattern block wrapper left in the post content within which to work.
Important notes:
syncStatusattrib set to still get wrapped in a pattern block. This was fixed in Pattern block: update frontend render code to match the new version of syncStatus attrib #50646Testing Instructions
Prerequisite: enable the Enhanced Patterns experiment toggle in the Gutenberg plugins experiment screen.
<!-- wp:pattern {"slug":"twentytwentythree/cta"} /--><!-- wp:pattern {"slug":"twentytwentythree/cta", "syncStatus":"partial"} /-->Patternblock wrapper around it, editing is set to contentOnly, but it does not set thewidealignment of its inner blocks on the pattern block itself.Screenshots or screencast
pattern-partial-sync.mp4