Block Bindings: Simplify block bindings object #58337
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] Code Quality
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/block-bindings/sources/pattern.php ❔ lib/compat/wordpress-6.5/block-bindings/sources/post-meta.php ❔ lib/compat/wordpress-6.5/blocks.php |
42 tasks
|
Size Change: -20 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Jan 29, 2024
| @@ -28,7 +28,7 @@ | |||
| } | |||
| }; | |||
| wp_block_bindings_register_source( | |||
| 'pattern_attributes', | |||
| 'core/pattern-attributes', | |||
There was a problem hiding this comment.
@talldan and @kevin940726, are you settled on the final name for the block binding source for Pattern Overrides? Tomorrow is the last day to change it 😄
There was a problem hiding this comment.
I'd vote for core/pattern-overrides but I wouldn't mind any other choices as long as it's consistent 😄.
There was a problem hiding this comment.
I agree I would try to keep it as consistent as possible. What is the name of the feature? Synced pattern overrides?
There was a problem hiding this comment.
I'm merging this pull request, but let's decide the name and we can change that in another pull request
There was a problem hiding this comment.
I'd also vote for core/pattern-overrides
There was a problem hiding this comment.
I've created this pull request to update that: link.
|
"Test image" section needs some updates to the HTML snippets included. Overall, all changes look good. I like the optimized format that @gaambo advocated for in #53300 (comment). We have been thinking about several options in the past few days, so this all helped to make the final decision. It would be great to have better e2e test coverage so we don't have to do all these tests manually. |
gziolo
reviewed
Jan 29, 2024
| continue; | ||
| } | ||
|
|
||
| $source_callback = $block_bindings_sources[ $binding_source['source']['name'] ]['apply']; | ||
| $source_callback = $block_bindings_sources[ $binding_source['source'] ]['apply']; |
There was a problem hiding this comment.
I think that we might need to introduce WP_Block_Binding_Source class so we don't have to do so many checks in the code. Here, we should check whether apply is defined with asset since it might not be as we don't don any validation at all.
There was a problem hiding this comment.
This could make sense 🤔 I've added that as a follow-up task in the tracking issue.
SantosGuillamot
requested review from
spacedmonkey,
ellatrix,
fluiddot,
dcalhoun,
Mamaduka,
artemiomorales,
michalczaplinski and
ajitbohra
as code owners
I just updated it. Thanks for pointing that out 🙂
I also feel the new format is cleaner!
Sure! It's part of the next steps defined for the upcoming weeks here. |
gziolo
added
the
Backport to Gutenberg RC
michalczaplinski
added a commit
to michalczaplinski/wordpress-develop
that referenced
this pull request
Jan 30, 2024
cbravobernal
pushed a commit
that referenced
this pull request
Jan 30, 2024
* Use namespaces in bindings sources * Change `value` for `key` in post meta source * Change `attributes` name for `args` * Add safety check in post meta source * Remove `source` level * Change comment * Adapt pattern overrides without `source`level
|
I just cherry-picked this PR to the release/17.6 branch to get it included in the next release: 9cf332a |
cbravobernal
removed
the
Backport to Gutenberg RC
youknowriad
pushed a commit
that referenced
this pull request
Jan 31, 2024
* Use namespaces in bindings sources * Change `value` for `key` in post meta source * Change `attributes` name for `args` * Add safety check in post meta source * Remove `source` level * Change comment * Adapt pattern overrides without `source`level
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
As explained here, the syntax of the block bindings object might change. I'm working on this pull request in case we want to follow that approach. The changes made are:
core/post-meta.valueforkey.attributesforargs.sourcelevel in the object.Why?
The new syntax could simplify and clarify the bindings definition.
How?
blocks.phpto this format.use-bindings-attributeshook used in the editor.Testing Instructions
In a page
Test paragraph
Test heading
Repeat the paragraph test but using a heading.
Test button
Test image
In a template
Go to a page template, for example, and repeat the process. In this case, the blocks can't show the value of the custom fields because it depends on each page. They should show a placeholder instead.
Test pattern syncing overrides