Image: Reflect media deletion in the editor #41220
Closed
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
ramonjd
added
[Type] Enhancement
|
Size Change: +7.21 kB (+1%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
This was referenced May 23, 2022
ramonjd
commented
May 23, 2022
| url: undefined, | ||
| id: undefined, | ||
| if ( id && ! isReplaced && ! isExternalImage( id, url ) ) { | ||
| isMediaFileDeleted( id ).then( ( isFileDeleted ) => { |
There was a problem hiding this comment.
I'm wondering if this will solve the reported issue in #41161 at all... if the image triggers the error handler, it could be due to a network error in which case the fetch request would fail.
🤔
There was a problem hiding this comment.
Is this a situation we could mock within some unit tests for the image block?
ramonjd
force-pushed
the
try/check-media-load-before-clearing-attributes
branch
from
cfa08c3
to
c54e532
Compare
|
Still pretty rough, but what do you think about the custom placeholder idea @glendaviesnz ? It's an extra step for the user, but at least they get to decide whether to clear it. |
…eleted from the attachments. If the image block was not replaced with an embed, and it's not an external image, and it's been deleted from the database, clear the attributes and trigger the placeholder.
ramonjd
force-pushed
the
try/check-media-load-before-clearing-attributes
branch
from
c54e532
to
f4ffb07
Compare
There was a problem hiding this comment.
Thanks for tackling this issue @ramonjd 👍
Most tests were successful but I did encounter one problem while testing "images that have been deleted".
After deleting an image but without refreshing the editor, when I attempt to replace the image for the first time, I get the image error placeholder again along with a new 404 in the dev tools console. If I attempt to replace the image a second time it works.
Screen.Recording.2022-07-11.at.4.26.23.pm.mp4
If after deleting the image, I refresh the editor as per test instructions then the initial attempt to replace the image succeeds.
Other than the issue mentioned above, I left a few other minor suggestions via inline comments.
| */ | ||
| export function isMediaDestroyed( id ) { | ||
| const attachment = window?.wp?.media?.attachment( id ) || {}; | ||
| return attachment.destroyed; |
There was a problem hiding this comment.
Should we force the return here to be boolean matching the JSDoc return type?
Suggested change
| return attachment.destroyed; | |
| return !! attachment.destroyed; |
| url: undefined, | ||
| id: undefined, | ||
| if ( id && ! isReplaced && ! isExternalImage( id, url ) ) { | ||
| isMediaFileDeleted( id ).then( ( isFileDeleted ) => { |
There was a problem hiding this comment.
Is this a situation we could mock within some unit tests for the image block?
|
@aaronrobertshaw I appreciate the review
Will take a look at this. The whole enterprise is a bit fiddly and am wondering if it's a house of cards. I'll try to stabilize it. |
|
I think I'll have to revisit this PR, and probably the UX. There's a bit of race going on between the time we check for a newly-selected image and when the There's an elegant way around this, but I don't know it yet 🤣 So I might put this PR on the back burner. I don't think there's a huge demand for it yet. Thanks again for the review. |
|
Closing this one for another time and place. |
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?
Another take at #35973 and #40982, which were reverted due to issues reported in #41161
Revert PR: #41221
This time, if the image block was not replaced with an embed, and it's not an external image, and it's been deleted from the database, we trigger a custom placeholder.
This gives the user the opportunity to:
Original issues:
Why?
The previous attempt was clearing the image automatically and did not take into account images external or otherwise that might be temporarily unavailable.
See the issue: #41161
props @eriktorsner
How?
Testing the suggestion, or my understanding of it, in #41161 (comment) from @danielbachhuber
Testing Instructions
For images that have been deleted
For images that haven't been deleted
For external images
cc @danielbachhuber whose idea I half-implemented 😄