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.
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: Disable editing of bound block attributes in editor UI #58085
Block Bindings: Disable editing of bound block attributes in editor UI #58085
Changes from all commits
2f071ad
57ab9b2
445405f
af7f870
c3567a4
35c8c64
989f456
a4dc34a
3515538
41709fa
1567f17
1984749
9644946
478f861
2acf6bd
a8a6da3
30b635e
a6f5fde
7d0cb9a
e6a5a4d
7c1ca5a
4246260
5a0cee8
54c313d
aaf8652
1f34e08
895e6dd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we also add the
linkTarget
here?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 were some issues with
linkTarget
. Let's keep it out of the scope for now.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.
Any idea or advance of how it's going to implement the
updateMetaValue
usage?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.
The technical implementation is almost there. However, there are many things to take into account, and it was decided not to rush things for the upcoming 6.5 to ensure that whatever we land is stable enough.
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.
Is it
metaValue
a proper name here? In theory, the source can be arbitrary meaning it could belong to metadata, but also to very different origins.Could we consider using something like
sourceValue
, andsetSourceValue
?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.
Good feedback. On the server, there is
get_value_callback
registered for every block bindings 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.
Is this a hook? You cannot use hooks inside conditions and loops
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.
In the case of post meta source, it uses
useSelect
anduseEntityProp
: link. Although each source could do whatever they want.If that's a problem, do you have any ideas on how that should be handled? We need to iterate through the different bindings and get the value from each 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.
The only solution is to probably mount sub components, get the result, and set some state in this component, similar to how it is done here:
gutenberg/packages/block-editor/src/hooks/utils.js
Line 537 in fcf0b5d
But I'd love to hear other people's thoughts
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.
Good catch @ellatrix! Yup, we'll have to change it because it is a hook.
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.
setAttributes
to have a stable reference between rerenders. This override breaks that.P.S. There is no need to wrap the returned
BlockEdit
in the fragment.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.
@Mamaduka, I think the issue at the moment is that users will only change the attribute when it is not bound. When the value is sourced from the block binding then the attribute becomes readonly. In effect, the batching isn't really necessary and even there should be no override set until it's possible to edit the external source in UI.
By the way, feel free to refactor the code here if you have some ideas how to optimize it.
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.
Is that handled somewhere else? Because I don't see that logic here. If the block calls the
setAttribute
override, it will update the attribute, bound or not.Sorry, I'm not super familiar with the internals of Block Binding API. I just came across this code while looking for something else.
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 is no way to trigger the update from UI at the moment for supported blocks and their selected attributes. Direct changes to the store don't matter because the value will get replaced on the frontend anyway.
You are totally right that batching is not needed at the moment and most likely the override for
setAttributes
is premature.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 confirming. I can push refactoring PR later today.
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.
What stops us from using the new "api" here? This will cause a bit of a performance regression.
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.
Is it possible to modify the
attributes
andsetAttributes
of theBlockEdit
component with the new API? If that's possible I believe I could change it.