-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Remove getPlaceholder
API and rely on key
argument or source label
#64910
Block Bindings: Remove getPlaceholder
API and rely on key
argument or source label
#64910
Conversation
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. |
bindings[ attributeName ].args?.key || | ||
source.label; |
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.
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
label
property 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.
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.
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
|
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.
LGTM
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 🎉 |
…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 <[email protected]> Co-authored-by: cbravobernal <[email protected]>
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. |
What?
Remove the current
getPlaceholder
API and rely instead on thekey
argument 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
getValues
defined (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
key
or the source label.How?
I'm removing the
getPlaceholder
API. Sources now decide what to show directly in thegetValues
functions. If the value isundefined
, it is up to them to decide if they want to passundefined
or other value like thekey
. For those sources withoutgetValues
defined, we default to thekey
or the source label if it exists.Testing Instructions
key
and the third one shows the source label.