-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add block bindings API basis and metadata source #56867
Add block bindings API basis and metadata source #56867
Conversation
$p2 = new WP_HTML_Tag_Processor( $markup ); | ||
$p2->next_tag(); | ||
$names = $p->get_attribute_names_with_prefix( '' ); | ||
foreach ( $names as $name ) { | ||
$p2->set_attribute( $name, $p->get_attribute( $name ) ); | ||
} | ||
return $p2->get_updated_html(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned in #56704 (comment) that this unfortunately disregards any markup before or after the found tag, so it won't work for button which is a <button>
inside a <div>
wrapper. It strips the <div>
wrapper.
I realize we need $p->set_inner_html
to support it properly, but in the meantime it might be worth removing button block support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right. I just made a commit that tries to make it work with heading and button structures: link. Whenever the set_inner_html
function is ready we can change the logic. But as of right now, we only plan to support a few core blocks. I don't see a problem having it hardcoded because we know their HTML structure.
Let me know if that works for you 🙂
if ( ! $p->next_tag( | ||
array( | ||
// TODO: build the query from CSS selector. | ||
'tag_name' => $block_type->attributes[ $block_attr ]['selector'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on #56704, this might not work for the heading block, but you could borrow from @kevin940726's approach in that PR of exploding the selector, at least until the HTML tag processor supports CSS selectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing! As part of this commit, I'm using that approach for supporting the different selectors we can have for the heading and the button.
return settings; | ||
} | ||
|
||
// TODO: Review the implications of this and the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what this code is doing? If the idea is to expose the context to all blocks that allow block bindings, then maybe it would work best to make these context entries global, or on the server they should be automatically injected during the block registration when the block allows binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two parts:
- Exposing the context to all blocks.
- Injecting the
<BlockBindingsUI />
component.
Regarding adding the context to all blocks I am not 100% sure if it is needed. Maybe sources like this one, can get the postType
and postID
without needed to add it to the block.json
. Any ideas if that's possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the implementation with this idea in this commit of another pull request (I've split this prototype in smaller PRs): link.
In the post meta source, I'm getting the postType
and the postId
using select( editorStore ).getCurrentPostId()
and select( editorStore ).getCurrentPostType()
when they are not defined. That way, we can decide to not show that source if we are in a taxonomy for example. And we don't have to register the context as we were doing.
Let me know what you think and I can reuse it in the rest of the sources 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I believe in that approach, we're not getting the context properly when we are in a query loop, for example.
As explained in this update, I've decided to split this prototype into smaller pull requests so they are easier to review and we can keep the conversations separately. Please feel free to jump there if you have any feedback 🙂 |
What?
Related issues:
In this pull request, I'm creating a first prototype of the Block Bindings API and the metadata source. Mainly covering:
I believe we can start with this prototype, after polishing and reviewing it, as an experiment so users can test it if wanted, and work on the following changes/features in other pull requests on top of it.
Why?
I prepared this demo to showcase what I tried to achieve and how the current prototype works:
Block.bindings.demo.mp4
In the video demo, after creating a "Movies" CPT and adding some custom fields like "poster URL", "release date", "runtime"..., I cover basically the UX:
How?
I prepared this video to explain how I did it:
https://www.loom.com/share/24ed54eaf5844db590fbe0a7d9ee324c
Video explanation
First, I'd like to recap how the block bindings API works. It basically:
1 . Add the "bindings" property to the block attributes in the editor.
This is probably the most challenging part and where more issues remain. What I did so far is:
blocks.registerBlockType
filter, add a "bindings" button to the blocks included in the whitelist and add the context needed for the metadata source.updateBlockBindingsAttribute
util that can be used by the different source to update the "bindings" property.getEntityRecord
, I'm fetching the available fields and showing them in the Block Bindings popover.2 . In the server, read the "bindings" property and get the value from the source specified there.
As part of this:
$block
,$block_content
, and$block_instance
in case they need that. For example, the "metadata" source will call theget_metadata
function, while a hypothetical "shortcode" source would usedo_shortcode
.render_block_
filter, whenever a block has a bindings property, it processes the object to get the value from the proper source and replace the proper HTML (next step).3 . Replace the HTML with the value obtained.
I created a first version of a function named
block_bindings_replace_html
to modify the HTML with the value obtained from the source. Depending on the block attribute source, the HTML to replace might be different. For example, for the paragraph content, with source "html", we want to replace the inner HTML. While with the image URL, with source "attribute" and "src", we want to replace thesrc
attribute.List of remaining decision/issues/features after this PR
After working on this property, it raised some decisions we have to take, some issues we have to solve, and some functionalities we might consider. This is a non-exhaustive list of what I believe could be the next steps.
As I mentioned, I believe we could work on them after merging this PR, and make changes on top of it.
I'll try to open new issues for the most relevant ones in the meantime I created this video explaining them
List.of.decisions_fixes.mp4
set_inner_markup
until that's ready.Testing Instructions