List View: Fix warning error when the gallery block has the same image URLs #53809
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
t-hamano
added
[Type] Code Quality
t-hamano
force-pushed
the
fix/list-view-same-url-images
branch
from
bb73960
to
a0387b7
Compare
|
Size Change: +13 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Aug 18, 2023
| @@ -194,7 +194,7 @@ function ListViewBlockSelectButton( | |||
| { images.map( ( image, index ) => ( | |||
| <span | |||
| className="block-editor-list-view-block-select-button__image" | |||
| key={ `img-${ image.url }` } | |||
| key={ index } | |||
There was a problem hiding this comment.
Using the index as a key isn't recommended. Maybe we could use clientId here.
There was a problem hiding this comment.
Thanks for the review!
Using the
indexas akeyisn't recommended.
I did not know about this 😅 I have taken the following actions:
- I have renamed the
getImageUrlfunction to thegetImagefunction. This is because this function returns an object containing information about the image. - Include the
clientIdin the return value of thegetImagefunction and use it as a key for the map function.
|
Flaky tests detected in 4863836. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5903964340
|
andrewserong
approved these changes
Aug 21, 2023
There was a problem hiding this comment.
Thanks for fixing this @t-hamano! Using the clientId sounds like a good way to keep the key unique for now 👍
In a follow-up, we'll likely be looking at creating some kind of API for exposing image urls from the block to the list view (i.e. to implement #53684). In that case, we might not want blocks to have to pass in a clientId (ideally a block would just return an image url) so we may need to change the logic slightly when we get to that. One possible approach could be to create a composite key using the URL + index, so that if either changes, a re-render will be triggered. But that's not a blocker for now, I think the requirements will become clearer once there's a WIP PR to play around with the API 🙂
In testing trunk, I can see the existing key fails for duplicate url keys, resulting in unintended duplicate rendering, most notable when moving child blocks. With this PR applied, with the Gallery block collapsed and then rearranging child images, the UI updates correctly and there are never more than 3 image thumbnails:
| Before | After |
|---|---|
 |
 |
LGTM! ✨
|
Thanks for the review, @andrewserong! Thanks for the information about the new API as well 🙇 |
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.
Related to #53728
What?
This PR fixes an error output to the browser console when opening a list view if the gallery block has images with the same URL.
Why?
This is because
keyprops are duplicated in themap()function.How?
Use
index. This change should have no impact on functionality, but if there is a problem, please let me know.Testing Instructions