-
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
Block Bindings: Use post meta label from register_meta
in block bindings workflows
#65099
Block Bindings: Use post meta label from register_meta
in block bindings workflows
#65099
Conversation
register_meta
in block bindings workflows
Size Change: +346 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
964b861
to
af79c2b
Compare
b5dc992
to
3464cdc
Compare
register_meta
in block bindings workflowsregister_meta
in block bindings workflows
I've been taking a look at the failing e2e tests and I believe it is a matter of rebasing to |
This reverts commit a43e391.
9b657e5
to
efa3b5d
Compare
Now that the base PR has been merged, and e2e are passing after rebasing, I believe this pull request is ready for review and potentially merge. The main changes affect the |
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @dannyreaktiv. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
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 left some initial feedback.
@@ -61,7 +61,7 @@ function BlockBindingsPanelDropdown( { fieldsList, attribute, binding } ) { | |||
{ registeredSources[ name ].label } | |||
</DropdownMenuV2.GroupLabel> | |||
) } | |||
{ Object.entries( fields ).map( ( [ key, value ] ) => ( | |||
{ Object.entries( fields ).map( ( [ key, args ] ) => ( | |||
<DropdownMenuV2.RadioItem | |||
key={ key } | |||
onChange={ () => |
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.
This part needs further enhancements to become more flexible. In particular:
[ attribute ]: {
source: name,
args: { key },
}
In my opinion, every source should construct args
argument, which for Post Meta happens to be { key }
. For Pattern Overrides that would be {}
or undefined
, and every other source could decide what that it.
In effect, I believe the object item from the fields
should have the following shape:
{
label: string,
value: string,
args: any,
}
This way it's up to the implementor to provide all that's necessary to make it work when defining getFieldsList
fir the source.
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 plan to create an issue to discuss the whole getFieldsList
API to ensure it feels good before making it public.
I believe I shouldn't have used the args
variable name here because it doesn't refer to the source args, but the object returned by getFieldsList
. There are two different things:
- The binding object: It specifies which attribute is connected to which source, which can already use any
args
they want. For example, in post meta we usekey
to specify which custom field the attribute is connected to:
"content": {
source: "core/post-meta",
args: { key: "my_custom_field" },
}
- The
getFieldsList
API: This is used just to get a list of ALL the bindable fields in order to show them in the UI and let the user select. In this case, I believe we only need thelabel
and thevalue
. As it is a callback, each source can decide what to show as the label and the value.
I hope we can clarify it by improving the getFieldsList
API (probably needs a new name) and that part of the code, because I agree it feels confusing right now.
What?
Fixes #65066
This pull request is built on top of #64072 because it reuses the
getRegisteredPostMeta
resolver.In this PR, I'm experimenting with using the label exposed there to consume it in the places where we are using block bindings. These are the use cases covered so far:
In the attributes panel
You can appreciate the difference both in the attributes section and in the dropdown.
In rich text placeholders for empty attributes
When the user can edit
When the user can not edit
When no label is provided, it still works as before
Why?
Right now, we are relying on the key, which is a bit technical. A recurrent feedback received is that it'd be nice to have a human-readable label.
How?
The main change done here is changing slightly how the
getFieldsList
API works (still private). Until now, it was returning an object of key: value. Now, I explore returning key: args. And including in those args both the label and the value. That way, it provides flexibility for each source. In the case of post meta, we get the value usinggetRegisteredPostMeta
resolver.Return from
getFieldsList
API:Testing Instructions