Block Bindings: Change placeholder when attribute is bound #64903
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] Bug
SantosGuillamot
commented
Aug 29, 2024
Comment on lines
188
to
190
| const _bindingsPlaceholder = | ||
| ( relevantBinding?.args?.key || blockBindingsSource?.label ) + | ||
| ' value is empty'; |
There was a problem hiding this comment.
It'd be great to get feedback on the desired text.
There was a problem hiding this comment.
I think is fine, maybe we can remove value.
There was a problem hiding this comment.
I thought about removing it, but when using the source label felt a bit weird:
- "my_custom_field is empty" sounds fine.
- "Custom Source is empty" doesn't sound as good to me.
Having said that, I don't have a strong opinion on this.
|
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 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. |
This was referenced Aug 29, 2024
|
Size Change: +1.42 kB (+0.08%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
cbravobernal
requested changes
Aug 30, 2024
Comment on lines
188
to
190
| const _bindingsPlaceholder = | ||
| ( relevantBinding?.args?.key || blockBindingsSource?.label ) + | ||
| ' value is empty'; |
There was a problem hiding this comment.
I think is fine, maybe we can remove value.
Co-authored-by: Carlos Bravo <[email protected]>
gziolo
reviewed
Aug 30, 2024
|
|
||
| const _bindingsPlaceholder = sprintf( | ||
| /* translators: %s: source label or key */ | ||
| __( '%s value is empty' ), |
There was a problem hiding this comment.
It should be a prompt for action, example:
Suggested change
| __( '%s value is empty' ), | |
| __( 'Type something to provide value for %s' ), |
It should be rather more concise.
There was a problem hiding this comment.
There are two aspects to consider here:
- To simplify the workflows and the implementation, my idea was to reuse the same "empty" message when the user can edit and can not edit the meta field. For example, we are showing this message when you bind a paragraph to an empty field in a query loop, where editing is disabled.
- This message appears in every rich text, including shorter blocks like "Button". With that in mind, I would like to keep it as short as possible.
Do you think that showing " X value is empty" is not enough for prompting the user to write? Do you think that we should show different messages when the user can edit or can not?
There was a problem hiding this comment.
Yes, these are two different use cases. In particular, these placeholder prompts calling for action were designed with accessibility in mind. It's best to check it with assistive technology to check whether it makes sense.
There was a problem hiding this comment.
Okay, I have just pushed a commit to differentiate between these use cases. It is now a matter of deciding the text we want to show for each case. What about this:
Use "Add {key} value" when the user can edit:
This follows what we are doing for post title or buttons for example. It is simple, but not sure if it is clear enough.
Use just the key (or label) when the editing is locked.
There was a problem hiding this comment.
Maybe just "Add empty_field"? Similar to add title, I think value is still not adding "value" 😆
There was a problem hiding this comment.
That could work for me 😄
| aria-readonly={ shouldDisableEditing } | ||
| { ...props } | ||
| aria-label={ | ||
| bindingsPlaceholder || props[ 'aria-label' ] || placeholder |
There was a problem hiding this comment.
is placeholder already defaulted on line 337?
There was a problem hiding this comment.
is placeholder already defaulted on line 337?
+1 — I think having just aria-label={ placeholder || props[ 'aria-label' ] } should be good.
There was a problem hiding this comment.
The problem is the priority. props[ 'aria-label' ] is supposed to override the default placeholder value if it exist for the aria-label attribute. And I believe we want to override the aria-label prop when a binding exists.
There was a problem hiding this comment.
@zaguiini, regarding your comment from #65089 (review).
Another issue, though related, is that the Block Bindings API does not respect an existing placeholder attribute. From the back-end, I'm setting a bespoke placeholder:
I think this is what changed the behavior, as placeholder became here a fallback. Now, the question is whether there is always placeholder coming from the server. If not, then we should be fine reverting to the old behavior and including the fallback to bindingsPlaceholder when the RichText is connected to a source.
| } | ||
|
|
||
| return _disableBoundBlocks; | ||
| const relevantBinding = blockBindings[ identifier ]; |
There was a problem hiding this comment.
I don't really like the name, but cannot find any better. Why is relevant?
There was a problem hiding this comment.
I find relevantBinding to be unclear.
Maybe relatedBinding could work? Another option is associatedBinding.
If neither of those, honestly for me just binding would work fine and might even be the clearest. Another possibility is richTextBinding (my least favorite, but could work).
Other ideas: connectedBinding, targetBinding, linkedBinding, though I like these options less and they also seem unclear to me.
There was a problem hiding this comment.
I can use relatedBinding 👍
| } | ||
|
|
||
| return _disableBoundBlocks; | ||
| const relevantBinding = blockBindings[ identifier ]; |
There was a problem hiding this comment.
I find relevantBinding to be unclear.
Maybe relatedBinding could work? Another option is associatedBinding.
If neither of those, honestly for me just binding would work fine and might even be the clearest. Another possibility is richTextBinding (my least favorite, but could work).
Other ideas: connectedBinding, targetBinding, linkedBinding, though I like these options less and they also seem unclear to me.
| aria-readonly={ shouldDisableEditing } | ||
| { ...props } | ||
| aria-label={ | ||
| bindingsPlaceholder || props[ 'aria-label' ] || placeholder |
There was a problem hiding this comment.
is placeholder already defaulted on line 337?
+1 — I think having just aria-label={ placeholder || props[ 'aria-label' ] } should be good.
|
I believe I've addressed the feedback you shared. This is how it is looking at this moment: Empty value when user CAN edit
Empty value when user CAN NOT edit
I believe we can explore adding a label to custom fields instead of using the key in future pull requests. Is this good enough? Something else I should change? |
cbravobernal
approved these changes
Sep 2, 2024
cbravobernal
deleted the
fix/change-placeholder-when-bound-value-is-empty
branch
14 tasks
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?
Adapt the placeholder and the
aria-labelattribute of the rich text component when the relevant attribute is connected to a block bindings source. The default texts don't make sense in this context.Same happens with the
aria-labeltext, which is also changed in this PR.Additionally, I took the opportunity to do a small refactor on
disableBoundBlock, which is used to lock editing when appropriate, to rely on the rich-text identifier instead of iterating through all the attributes.Why?
It is confusing to get a prompt to insert a block when user is actually editing the binding source value.
How?
Return an alternative placeholder when the attribute used as identifier in the RichText is connected to any source. And use this placeholder when it exists.
Testing Instructions
aria-labelhas also changed.