Block Bindings: Remove getPlaceholder API and rely on key argument or source label
#64910
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] Enhancement
|
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. |
SantosGuillamot
commented
Aug 29, 2024
Comment on lines
186
to
187
| bindings[ attributeName ].args?.key || | ||
| source.label; |
There was a problem hiding this comment.
Here, I'm using just the key if it exists and the source label if it doesn't. The key has been reported to be too technical. From past conversations, there are other options to consider:
- Use always the source label. However, it could be confusing when there are multiple blocks connected to the same source but different keys. For example, I can see a common use case in templates.
- Use something more verbose like Post Meta "my_custom_field". The downside is that it could be too much.
- Add a new
labelproperty in the fields registration. This seems the best option to me, but it might take time so, if we decide to take that approach, I believe it could be done in a follow-up pull request.
I'd love to get more feedback on this.
|
Size Change: +1.27 kB (+0.07%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
|
Thinking a bit more about this, I believe
This way, we keep solving the issues we were facing, we don't expose any placeholders APIs we aren't sure about yet, we let sources decide to return Let me know what you think about it. |
|
Flaky tests detected in 8acbdcc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10619607033
|
64 tasks
I think it is flexible enough. We may need to document it, as developers will find difficult to the discover that the order is: If the source is not setting any label, the binding is not working. And there is PHP notice asking for it.
|
I would say that is expected because registered source must contain a label: link. |
Yep, but also nothing crashes 🎉 |
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
…t or source label (WordPress#64910) * Remove `getPlaceholder` API * Skip unregistered sources * Add e2e tests * Accept undefined as a valid value * Properly check if value is a URL Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
|
Now that this got merged, the blocks will always have a predefined value, which is OK, but how can I remove that initial value? For example, in the Create Content Model project we're defining a nice placeholder in CPT definition:
But that placeholder value is optional. If it's empty, on data entry, we should display this placeholder:
However, this is not the case, because the meta key gets displayed instead:
The same issue is observed in the other attributes:
It seems to me that we should keep the attribute empty. The key as the default value is a bit misleading because there is no actual value saved in the post meta. 🤔 At the very least this behavior should be filterable (like this, but for the placeholder, and within the editor), even for existing sources. |
|
If I am not mistaken, this pull request shouldn't have changed that behavior. Previously, we were also showing the Let me share how it is supposed to work to see if we are aligned. There are different use cases:
In the future, all the meta keys will potentially be changed by a label if that's added to meta registration as suggested here. Is this not aligned with your expectations? If something not working like this, could you describe the use case, please? |
|
I was able to reproduce the issue that @zaguiini reported. When I remove the text for the Button connected with poste meta, then it gets replaced with the key of the field. Instead, it should leave the value empty and change the placeholder text which for regular block looks as follows: 
Connected block when the value gets removed: 
I suspect, based on the screenshots shared by @zaguiini, that it's the case everywhere as there is no reason to see any values in the sidebar in textarea and input fields when the post meta value is empty. This is how it looks for regular blocks: 
and when I connect the field and remove the value for the 
|
|
I have just created this pull request aiming to solve that issue. |
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?
Remove the current
getPlaceholderAPI and rely instead on thekeyargument if it exists or in the source label if it doesn't.Some examples where placeholders are needed could be if we are in a template or if we are consuming a source that is registered only in the server.
Additionally, I added some logic to use the same placeholder (key or label), in sources without
getValuesdefined (sources defined only in the server, for example). Previously, they were showing the original attribute value, which could be confusing.Why?
It isn't clear if an API for the placeholders is needed or how it should look like. So far, it seems to be enough relying on the
keyor the source label.How?
I'm removing the
getPlaceholderAPI. Sources now decide what to show directly in thegetValuesfunctions. If the value isundefined, it is up to them to decide if they want to passundefinedor other value like thekey. For those sources withoutgetValuesdefined, we default to thekeyor the source label if it exists.Testing Instructions
keyand the third one shows the source label.