Add __default binding for pattern overrides
#60694
Conversation
kevin940726
added
[Feature] Synced Patterns
|
Size Change: +109 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The code here is mostly looking good, left a few minor comments.
I guess one consideration is whether __default is a pattern overrides feature or a block bindings one (other sources can use it). At the moment this implementation is for pattern overrides only, which I think is ok to start with as it means there's no need for backwards compatibility if we change our minds.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
kevin940726
force-pushed
the
add/default-pattern-overrides-bindings
branch
from
8015b8f to
1e7b666
Compare
| if ( hasPatternOverrides ) { | ||
| newAttributes.metadata.bindings.__default = { | ||
| source: 'core/pattern-overrides', | ||
| }; | ||
| } |
There was a problem hiding this comment.
This works ok in the site editor, but for some reason isn't working in the post editor when navigating to the original pattern. It must be that convertLegacyBlockNameAndAttributes is never called when switching to edit a different entity.
There was a problem hiding this comment.
This works ok in the site editor, but for some reason isn't working in the post editor when navigating to the original pattern.
Could you clarify the testing instructions for this? I'm seeing a consistent result on my end.
Kapture.2024-04-23.at.14.59.09.mp4
It won't automatically change from content to __default as shown in the recording, but I think it's expected as the function only runs on change. It's also backward compatible this way, I believe. Maybe there's something I'm missing though 🤔.
|
Have you considered using |
Yeah, the
I'd imagine that those blocks would have the attributes setting opt-in to support this feature.
I don't know really, it depends on the direction of the Block Binding API we want to take. I think the extra processing is pretty minimal as we already require some processing. It'll short circuit if the block doesn't have the |
|
@SantosGuillamot, @artemiomorales and @youknowriad, any thought on adding |
On one hand, I feel this is currently only needed by pattern overrides and I can't think of another use case which would use it. On the other hand, if we want to use the block bindings APIs for pattern overrides in the editor as discussed here, we will probably need to support it for all sources/blocks. For example, if I have an overridable paragraph content inside a pattern:
|
The current way it works in this PR is that it'll map |
|
I feel like I lack a lot of context in this PR. What is |
The problem is that if we move to use the block bindings APIs in the editor, we read the |
The issue #58601 is linked in the PR description that has more context. I can copy and quote it here for better visibility though. In short, we want a backward-compatible way to support more attributes as we move forward. Currently we have to list them all in the markup, but the UI only allows us to toggle them all at once. This means if we add a new supported attribute it won't be automatically synced/connected. Hence, a |
Oh yep, I don't have much knowledge of that implementation yet, unfortunately 🤔. I think it's still pretty unclear how #60721 would work with pattern overrides, and we haven't seen any follow-ups on that either. I think it's something we need to keep in mind when implementing it using |
| ) { | ||
| const bindings = [ | ||
| 'content', | ||
| 'url', | ||
| 'title', | ||
| 'id', |
There was a problem hiding this comment.
To do this strictly correctly, we should use PARTIAL_SYNCING_SUPPORTED_BLOCKS to match attributes to specific block types. Right now, this function is duplicating what is already some temporary hardcoded information, which just seems a bit too error-prone.
That said, IMO the strictly correct way seems overkill for the purpose of this branch. We could just loop over all keys of .metadata.bindings to replace .source.name and call it a day.
There was a problem hiding this comment.
I agree we should probably just iterate through the bindings and remove the ones with source core/pattern-overrides, and avoid having this hardcoded list.
| $supported_block_attrs = array( | ||
| 'core/paragraph' => array( 'content' ), | ||
| 'core/heading' => array( 'content' ), | ||
| 'core/image' => array( 'id', 'url', 'title', 'alt' ), | ||
| 'core/button' => array( 'url', 'text', 'linkTarget', 'rel' ), | ||
| ); |
There was a problem hiding this comment.
I understand that right now things are in flux and it's more convenient to hardcode or keep certain code paths private, but what's the plan for these mappings in the future?
- An attribute property in the schema?
{ "attributes:" { "content": { "...": "...", "overridable": true } } } - A block-supports record?
{ "supports": { "overrides": [ "content" ] } }? - Something entirely different?
There was a problem hiding this comment.
I think we've briefly discussed this before but decided to leave this for the future when we have a clear understanding of what the Block Binding API would look like. I believe the general consensus for the first version of pattern overrides is to use a hard-coded list.
|
Why not just inject the "bindings" attribute dynamically in the backend like we tried before? Why hard code it when you toggle the "pattern overrides". We won't need the |
@youknowriad Since #60234, pattern overrides are now opt-in, so both a name and something additional to indicate the opt-in are required. In trunk, the name and the per-attribute bindings are set, but those are not very future proof because of the issues mentioned previously. Here the The rest of this PR does inject the bindings dynamically the same way as that other PR, but only does so when the
Thanks for flagging that. I guess we need to make sure the two work well together first before moving forwards with this PR. It'd probably be best to focus efforts on #60721 first in that case. |
…e` function. Default should never be passed in to this function.
…more generic (not implemented in the pattern block)
talldan
force-pushed
the
add/default-pattern-overrides-bindings
branch
from
b10a29c to
869d53d
Compare
|
I've pushed a PHP backport in WordPress/wordpress-develop#6694, though I still need to work out the best way to unit test the change. The test I've added currently isn't passing. |
|
@SantosGuillamot @gziolo I've rebased the PR, addressed those comments, and there's now a PHP backport too. Hopefully this is ready to go. |
There was a problem hiding this comment.
From my tests, everything seems to keep working 🙂 Aside from the open discussions I mentioned here, I don't have any more comments.
|
Excellent, thank you @talldan for fixing the potential issues with the filters around the Synced Pattern block (or whatever |
bph
added
[Type] Enhancement
Co-authored-by: kevin940726 <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: mcsf <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: youknowriad <[email protected]>
Co-authored-by: kevin940726 <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: mcsf <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: youknowriad <[email protected]>
What?
Part of #59819, close #58601. Alternative to #59956.
Why?
See #58601.
How?
Add a special
__defaultbinding for pattern overrides as suggested in #60066 (comment).__defaultis a way to communicate that we want to connect all supported attributes to a source (pattern overrides.) So that when we add support for new attributes the pattern will be automatically connected.Testing Instructions
Screenshots or screencast
N/A