MediaReplaceFlow: Fix UI showing stale URL by avoiding state duplication #42116
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
mcsf
added
[Type] Bug
|
Size Change: +695 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Jul 4, 2022
Comment on lines
64
to
68
| useEffect( () => { | ||
| if ( mediaURL && mediaURL !== mediaURLValue ) { | ||
| setMediaURLValue( mediaURL ); | ||
| } | ||
| }, [ mediaURL ] ); |
There was a problem hiding this comment.
The new React docs recommend adjusting the state while rendering for similar cases. But I think this might be easier to understand at a glance.
There was a problem hiding this comment.
Thanks for the reference! I don't think this was in the recommended practices a couple of years ago, but it makes a lot of sense nowadays, especially in the context of concurrent rendering. :)
Mamaduka
approved these changes
Jul 4, 2022
|
@Mamaduka: I was going to give your feedback a go, but then I decided to have a quick look at how easily one could just remove the internal state. Well, I have some hope that a7a776a is actually enough. I've manually tested it with block types Image, File, Gallery, Video, and Cover (FYI, blocks like File and Cover don't even show the URL in the popover, because they don't use MediaReplaceFlow's Let's see if the automated tests raise any flags. In the meantime, a sanity check would be welcome. :) Edit: in 31a0893 I updated a unit test for the component to reflect the fact that there needs to be a wrapper component that keeps the state of |
mcsf
changed the title
|
Thanks for the follow-up, @mcsf! This tests well for me 👍 |
draganescu
approved these changes
Jul 5, 2022
|
Thank you both! |
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?
Fixes a UI bug discovered in #41994.
Why?
After a new Image block has uploaded an image to the media library, the "Replace" popover will incorrectly report the image's URL as
blob:..., even though the block has correctly updated itsurlattribute to the new resource in the media library.How?
The
MediaReplaceFlowcomponent is passedmediaURLas a prop, but it also keeps its own internal URL state (mediaURLValue) usingmediaURLas a seed. The reason for keeping internal state is to let users edit the URL in an "edit and confirm" kind of interaction, as initially implemented in #16200 (see line), but that lends itself to synchronisation bugs. This PR removes the component's internal state altogether and uses themediaURLprop directly. The actual link-editing functionality is provided byLinkControl(a descendant ofMediaReplaceFlow), which already does it own state synchronisation anyway.Testing Instructions
See original report in #41994 (comment).
blob:URL.