Block Bindings: Disable editing of bound block attributes in editor UI #58085
Conversation
SantosGuillamot
requested review from
ellatrix,
fluiddot,
dcalhoun,
artemiomorales,
michalczaplinski and
ajitbohra
as code owners
SantosGuillamot
added
[Type] Feature
|
Size Change: +1.34 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1f34e08. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7639951981
|
artemiomorales
changed the title
artemiomorales
added
[Type] New API
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Looking good, thanks @SantosGuillamot ! A couple suggestions:
I don't see any design blockers for 6.5 here. During beta testing we might pick up the need to better highlight blocks that are connected to a field in some way. |
|
There are e2e test failures logged for Pattern Overrides so it needs to be investigated. I suspect, it’s no longer possible to edit content for the blocks that have overrides enabled in the pattern. |
| @@ -376,7 +376,7 @@ export function RichTextWrapper( | |||
| useFirefoxCompat(), | |||
| anchorRef, | |||
| ] ) } | |||
| contentEditable={ true } | |||
| contentEditable={ props.isContentBound ? false : true } | |||
There was a problem hiding this comment.
@ellatrix, what alternatives are there to put RichText in read-only mode that don't require introducing a new prop?
I'm also wondering what should happen for all these users using assistive technology. It feels like we should ensure it still possible to move focus when tabbing to the HTML element as in some cases RichText is the block wrapper so it could make it harder navigating to blocks like Paragraph or Heading.
There was a problem hiding this comment.
Noting also that this new prop fails one of the unit tests:
There was a problem hiding this comment.
I believe we can get directly the block attributes from the RichText component and read the metadata.bindings there. I'll give it a try.
There was a problem hiding this comment.
I made this commit to reflect what I was referring to. It seems cleaner to me, but I am not familiar enough with RichText to say if that's the right approach.
There was a problem hiding this comment.
I also notice that selecting the Paragraph block that is connected to a source requires a double click in the "edit" mode. In the "select" mode it seems that it works just fine.
output_56e741.mp4
There was a problem hiding this comment.
I agree it's not the best UX. I think that's the way the RichText works, right now when the editing is locked, so I am not sure if that's on purpose or we should look for a different approach.
There was a problem hiding this comment.
This should be fixed with this commit.
| @@ -296,7 +298,7 @@ function ButtonEdit( props ) { | |||
| onClick={ startEditing } | |||
| /> | |||
| ) } | |||
| { isURLSet && isLinkTag && ( | |||
| { isURLSet && isLinkTag && ! metadata?.bindings?.url && ( | |||
There was a problem hiding this comment.
What if someone would like to unlink the button? In general, should it be possible to remove the binding in the UI?
There was a problem hiding this comment.
In general, should it be possible to remove the binding in the UI?
I wasn't sure about that one. My reasoning was that, as right now bindings can only be added through the code editor, it shouldn't be a blocker if they can only be removed through the code editor as well. When we improve the UI and add the possibility to create bindings, we should provide mechanisms to remove them as well.
Anyway, I don't know which is the correct approach. Any thoughts?
|
|
||
| // Prettify the name until the label is available in the REST API endpoint. | ||
| const keyToLabel = ( key ) => { | ||
| return key |
There was a problem hiding this comment.
It isn’t the most user friendly to show in the UI machine readable key that can’t be localized. Maybe it’s fine for someone technical creating the site, but far from perfect for someone editing the content.
There was a problem hiding this comment.
I agree. I saw that as a workaround. The issue is that we don't have labels in the meta fields yet. Do you think it would be better to just pass the label? text_custom_field for example.
There was a problem hiding this comment.
Maybe that would be more appropriate as it would be clear it’s a code name. At least until we support labels. @SaxonF, what do you think?
There was a problem hiding this comment.
I was considering using something like "Connected to { meta_key } from { source }". Which would translate into "Connected to my_custom_field from Post Meta". Maybe it is too long for some cases like a button text.
Could you share the serialized block to test it? Maybe you are connecting to a custom field that doesn't exist in the context of the homepage, and that's why the blocks are not rendering properly. Anyway, if that's an issue, I believe it should be related to the PHP logic.
Makes sense. I was using the
I wanted to explore that option as well. There is a challenge because the options in the sidebar don't appear until an image has been uploaded. But we need to figure out how to solve that.
I agree 🙂 I believe it would be nice to land a simpler UI first to have some testing but we can iterate over it during the upcoming weeks.
Yep, I still have to take a deeper look at how it integrates with patterns because it might break things. Maybe we can check if the |
|
I think I’ve addressed most of the feedback, especially the blockers. There are some missing decisions/improvements, but I believe most of them can be done in other pull requests. These are the things I identified:
Would you consider any of those a blocker? Is there anything else I’m missing? Once this pull request is merged, I can start working on the points in this list. |
|
I see an issue: The content of the block is not updated when you edit the metadata in the block. You have to make some other edits in order for the content to be updated on the frontend. In the editor, the value is updated correctly. output_3b7505.mp4 |
I can reproduce the same issue with the Image block. |
|
I've been trying to reproduce it following the exact same process, but I haven't been lucky. I'll keep trying to be able to identify the issue better. |
Hmm, am unable to reproduce this one following the same steps 🤔 |
There was a problem hiding this comment.
I'm approving this PR on the grounds that removing the experimental flag has already been merged and the premise of this PR, which is to disable editing of bound fields and improve the UX, is working as intended as far as I can see.
I think we can continue addressing the improvements Mario outlined as well as fix any outstanding issues as feedback comes in.
| 'core/paragraph': [ 'content' ], | ||
| 'core/heading': [ 'content' ], | ||
| 'core/image': [ 'url', 'title', 'alt' ], | ||
| 'core/button': [ 'url', 'text' ], |
There was a problem hiding this comment.
Should we also add the linkTarget here?
There was a problem hiding this comment.
There were some issues with linkTarget. Let's keep it out of the scope for now.
This was caused by the recent changes in In summary, the issue should not be present in |
| const { | ||
| placeholder, | ||
| useValue: [ metaValue = null ] = [], | ||
| } = source.useSource( |
There was a problem hiding this comment.
Is this a hook? You cannot use hooks inside conditions and loops
There was a problem hiding this comment.
In the case of post meta source, it uses useSelect and useEntityProp: 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.
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:
But I'd love to hear other people's thoughts
There was a problem hiding this comment.
Good catch @ellatrix! Yup, we'll have to change it because it is a hook.
| ); | ||
|
|
||
| if ( source ) { | ||
| // Second argument (`updateMetaValue`) will be used to update the value in the future. |
There was a problem hiding this comment.
Any idea or advance of how it's going to implement the updateMetaValue usage?
There was a problem hiding this comment.
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.
| // Second argument (`updateMetaValue`) will be used to update the value in the future. | ||
| const { | ||
| placeholder, | ||
| useValue: [ metaValue = null ] = [], |
There was a problem hiding this comment.
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, and setSourceValue?
There was a problem hiding this comment.
Good feedback. On the server, there is get_value_callback registered for every block bindings source.
| setAttributes={ ( newAttributes, blockId ) => | ||
| registry.batch( () => | ||
| updateBlockAttributes( blockId, newAttributes ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
- The blocks expect
setAttributesto have a stable reference between rerenders. This override breaks that. - What's the reason for batching the single action? Technically, it shouldn't make a difference.
P.S. There is no need to wrap the returned BlockEdit in the fragment.
There was a problem hiding this comment.
@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.
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.
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.
it will update the attribute, bound or not
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.
Thanks for confirming. I can push refactoring PR later today.
What?
As discussed in this Slack thread, I'm splitting this other pull request to handle first just the use case where the editor understands the bindings attribute and adapt the UI accordingly.
As part of that, in this PR I'm doing:
BlockEditto modifyattributesandsetAttributesand make it understand the bindings: link.Why?
Splitting the original pull request should make it easier to review and it could help us land something sooner that covers understanding the bindings from the editor, although it doesn't include any UI to create/modify the bindings.
Testing Instructions
For all the testing, we have to go to Gutenberg-Experiments and enable the Test Block Bindings option.
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.
Remaining issues
I'll try to gather here the remaining issues related to this pull request. So far I'd like to work on:
Screenshots or screencast
Understanding.Bindings.in.the.Editor.mp4