Block Bindings: Fix empty strings placeholders in post meta bindings #65089
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
requested review from
gziolo,
artemiomorales,
zaguiini and
cbravobernal
SantosGuillamot
requested review from
ntwb,
nerrad and
ajitbohra
as code owners
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @zaguiini. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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
added
[Type] Bug
|
Size Change: +11 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
zaguiini
approved these changes
Sep 5, 2024
There was a problem hiding this comment.
The bug is fixed 👍🏻
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:
Can we preserve the existing placeholder attribute if existing? Do we want to address that in a separate PR?
I wasn't sure what the expected behavior was in that use case. I would prefer having a separate issue or PR to discuss the different solutions. |
cbravobernal
deleted the
fix/accept-empty-strings-in-post-meta-bindings
branch
|
I would respect the placeholder value provided on the server through filters and give it top |
|
@gziolo I'd appreciate some pointers here as I haven't worked much on the Bindings API. That is, of course, if @SantosGuillamot doesn't have the bandwidth to work on it. |
|
I'm gonna be away for a week, so if the plan is to include that for the next Gutenberg release, I won't have the bandwidth 😄 It could be even considered a bug though, so maybe it can get included during beta period. Regarding the code, placholder logic is happening here. |
This was referenced Sep 26, 2024
This was referenced Sep 28, 2024
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?
As reported here, the changes introduced to properly show placeholder in empty custom fields broke after that PR.
This PR addresses that and introduces an e2e test to avoid having the same issue in the future.
Why?
It is broken.
How?
I just changed the conditional where we change the value to consider an empty string as a valid value.
Testing Instructions
defaultempty.