Fix upload button on overridden empty image block in patterns #59169
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
kevin940726
added
[Type] Bug
|
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 pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/blocks.php |
talldan
added
the
Backport to WP 6.7 Beta/RC
|
Size Change: +7 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6a8ceb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7956496741
|
aaronrobertshaw
approved these changes
Feb 20, 2024
There was a problem hiding this comment.
This tests well for me @kevin940726 👌
In early explorations (#53887) I found the id needed to be tracked as well to make sure when replacing an image via the media library, the correct original selection was shown.
I can replicate that issue of the missing image selection in the media library on trunk and confirm that this PR fixes it as well.
✅ Upload button is no longer incorrectly shown
✅ Crop button now displays correctly
✅ Image selection within media library is now correct
| Before | After |
|---|---|
 |
 |
|
@SantosGuillamot @gziolo @michalczaplinski @artemiomorales Just a heads up that this adds the image block's |
|
I notice this caused a PHP error due to the So I think along with this change the processing code would also need to skip attributes without a source. |
This was referenced Feb 20, 2024
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
Co-authored-by: kevin940726 <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: talldan <[email protected]>
|
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: 4fcb480 |
getdave
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
Co-authored-by: kevin940726 <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: talldan <[email protected]>
gziolo
reviewed
Feb 21, 2024
| @@ -31,6 +31,7 @@ export const PARTIAL_SYNCING_SUPPORTED_BLOCKS = { | |||
| rel: __( 'Link Relationship' ), | |||
| }, | |||
| 'core/image': { | |||
| id: __( 'Image ID' ), | |||
There was a problem hiding this comment.
There is a note in the code:
// TODO: This should not be hardcoded. Maybe there should be a config and/or an UI.@talldan and @SantosGuillamot, I contemplated the other day whether we should add labels to block attributes schema and annotate them as translatable in block.json. That could also be considered as one of the conditions to opt into block bindings in the future.
There was a problem hiding this comment.
By the way, I think you should use _x() with the context so it's easier to translate these labels.
gutenberg/packages/blocks/src/api/registration.js
Lines 271 to 274 in a04a8e9
The context could be similar to:
block attribute label could work. That simpler part might make sense to update still before WP 6.5 beta 3.
There was a problem hiding this comment.
Yeah, I always have some doubts about including the translation strings here. Currently, they are not used in any way but are still kept. Should we just remove them for this release to not cause confusion for translators?
There was a problem hiding this comment.
Let's remove them to prevent unnecessary work for translators. 16 most popular locales need to have 100% coverage before the major release starts 😅
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?
Fix #58708.
Fix the confusing upload button on the block toolbar for overridden empty image in patterns.
Why?
The upload button is confusing and it creates bad UX.
How?
Add
idas a binding attribute for the image block as well.Testing Instructions
Follow the testing instructions in #58708.
Also added an e2e test.
Screenshots or screencast
N/A