Pattern block: Update block to retain wrapper #50272
Conversation
|
Size Change: +700 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
glendaviesnz
requested review from
spacedmonkey,
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
The decision to remove the parent selector for This makes it tricky in terms of the Pattern block for making it easy for user to get to parent to remove template locking. Should we add something to the Inspector panel instead? |
There's a bit of inconsistency in content only mode, in that List View only shows the Pattern block, the breadcrumbs still shows all blocks and the missing parent selector makes it difficult to select any parent (there's no reason a pattern couldn't be deeply nested). Breadcrumbs feels like it could be updated to remove non-content blocks. List View could potentially still show the child content blocks (I think @noisysocks explored this in #50082). I'm not sure what the right way forward is for the parent selector, whether it could select the pattern or the parent of the pattern, but I'm not sure it should be completely missing. It might be worth spinning this out into a separate issue to get some design feedback. |
|
Hello there! and thanks for exploring the pattern block updates. I wanted to ask about the choice to actually update the "pattern block" instead of planning to add the features mentioned above (shuffling, retaining reference to the original pattern, ...) to the "Group" block or create a new dedicated block instead. The reason I'm asking this is because the current pattern block is actually meant for something completely different and it's really unclear to me whether it should be used as the "pattern" block described in the linked issue. For instance, just the fact of adding a wrapper to the pattern block is going to be a breaking changes for all themes using it. We've discussed the pattern block some time ago with @mtias and I believe he was of the opinion of having a dedicated block for this while I was more of the opinion of having "metadata" added to any group block (potentially a metadata block support). I can be onboard with any of these two options (especially if we start as an experiment to assess) but to be honest using the current pattern block for this work seems too much of a stretch for me. It's just the name that is confusing. |
I think it was just the first and easiest way to experiment given the block only needed a few adjustments, and the overview issue guided it this way. I was also interested in the idea of using a block supports setting for container blocks. I did start hacking away at it briefly a while back. I straight away found it is a little tricky to add this mode arbitrarily to a block—we need to be able to inject the pattern's inner blocks and contentOnly mode into the block's edit, or maybe completely replace the edit component with a simplified contentOnly version. I could try to revive that if you think it's worth it? I do think it has some promise, one thought I had is that it might evolve towards a more unified way for building synced blocks. Also it potentially solves the issue of making pattern block alignments work given there would be no wrapping block. Probably the downsides are that if it's applied to any block, there are more likely to be edge cases where a block has an unusual implementation. Support for third party blocks as wrapping block in patterns might also be more complicated. |
yeah, I think, that's a valid concern, I don't think we should be applying to any block. Just one but some "metadata" can apply to any block: like naming blocks, marking it as "synced" or not, adding semantics (tagging).
Yes, I think this should be our North Star. It also makes me wonder whether the wp/block (reusable block) is actually the right block for these (but it also has history similar to the pattern block) Anyway, I don't want to force any direction or anything, I'm happy for this to be explored in any way (new block, pattern block, reusable block, group block). I just think we should be clear that it's an experiment to start with and that we stay open and adapt it down the road as we add "features" to these blocks. |
|
I have added a very draft PR here that adds a slug attrib to the Group block just to further explore the possibilities, but personally, it seems like adding this additional functionality to a Group, or some other wrapper block, could end up confusing users wondering why the Group behaves so differently depending on whether it is just a Group, or whether it is acting as pattern/reusable block wrapper. |
I think using the reusable block is interesting too. I had a go at implementing it in #50309. This kind of consolidation could also potentially work for template parts in the future. |
|
The following feedback is mostly based on the alternative PR by @talldan but wanted to keep the discussion in one place:
Should we leave the I also noticed that now there's a "ref" and "slug" props in the reusable block and it made me about the impact of the choices we're making now. As we try to unify everything, I feel like we should try to unify the props first. I'm thinking like right now it's the couple "type + ref" or "type + slug" that defines the behavior. It doesn't seem ideal, what would we do if we were to start from scratch:
Also, what's the difference between "template part", "reusable block" and "pattern" if the three can be saved/synced and referenced. The answer is that it's a combination of:
This is to say that ultimately, it seems the ideal scenario is to have a unique block let's assume it's
the wp_block CPT could have a property to define what capabilities are needed to edit such block. For the current PR, of course, we're not trying to solve everything, I guess what I'm trying to say is why do we automatically convert blocks and automatically assume that it's "contentOnly" locking. Should we instead start by making these things explicit in the block itself: add the syncStrategy attribute to the block and switch behavior based on the attribute instead. |
|
I am going to close this for now as we are more likely to follow the approach taken here. I have linked to the comments here on this issue so they don't get lost. |
Yes, maybe. I wanted to get an idea of what unifying some concepts would look like to see if it could be the path forwards. @gziolo also mentioned privately that we might do the reverse and instead migrate everything to the pattern block. I think the idea being that 'wp-pattern' is a better name for what's trying to be achieved than 'wp-block'.
I do wonder if we would make reusable blocks use a slug (like template parts) instead of an id. This is one of the things I had in mind when thinking about a
Would you be able to share more detail on what your thoughts are in terms of how they get into the database? I guess there are two options:
Perhaps to start with it's just a single endpoint that can retrieve everything, which again seems similar to how template parts work.
I think how this works is becoming clearer in my mind, with a few technical points that need clarity, so I think I'll try documenting it in a discussion or RFC style issue. |
In my mind, they'd just use the wp_block CPT, the only difference is how to "sync" them. So in other words, they're just reusable blocks with a new block attribute. I think for a start, maybe it's better to forget about the "theme" entirely, and focus on the block, how it should behave. The rest is a tangeant due to the WP context. |
… appear on pattern block (#50118) --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
…ning the inner blocks
glendaviesnz
force-pushed
the
update/pattern-block-to-retain-wrapper
branch
from
aaab236 to
177b85a
Compare
|
Flaky tests detected in 177b85a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4942577898
|
… === partial" This reverts commit e70dc51.
|
To avoid confusion reverted latest change and closing in favour of #50533 |
What?
If the syncStatus attribute of a pattern is set to
partiala pattern block wrapper is added and and templateLock is set to `contentOnlyeWhy?
How?
If pattern has
syncStatus===partialadds a parent div that wraps the content of the pattern. Also currently defaults the innerBlocks templateLock tocontentOnly.Testing Instructions
<!-- wp:pattern {"slug":"twentytwentythree/cta"} --><!-- wp:pattern {"slug":"twentytwentythree/cta", "syncStatus":"partial"} -->widewidth) and hasPatternblock wrapper around it, and editing is set to contentOnlywidewidth)Screenshots or screencast
pattern-partial-sync.mp4