-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add loading state on image upload in featured image, Site logo and Cover blocks #59519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. |
|
@t-hamano Can you please review this PR ? |
|
@Mamaduka need your help to review this PR. |
|
@sunil25393, I'll try to have a look next week. Side note: It's usually easier for review to review atomic PRs targeting a single block or feature. |
I understand, but since the code is for similar kind of functionality for all three blocks, i included it in single PR. |
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @sunil25393!
I left a couple of notes based on the initial review.
| @@ -1,4 +1,6 @@ | |||
| .wp-block-cover { | |||
| position: relative; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this rule was removed for some reason after the loading state was introduced for the Cover block (#25401).
Let's avoid unexpected regression and only apply this style conditionally. We can put &.is-transient { position: relative; } near other loading state classes (they start at L32).
| // When image being uploaded. | ||
| &.is-transient img { | ||
| opacity: 0.3; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the &.is-transient::before styles already handle this, so there's no need for image opacity.
| // Reset temporary url when media is available. | ||
| useEffect( () => { | ||
| if ( media && temporaryURL ) { | ||
| setTemporaryURL(); | ||
| } | ||
| }, [ media, temporaryURL ] ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can ditch this effect and reset setTemporaryURL in onSelectImage when the feature image ID is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the same thing before, however it takes time to set media after calling onSelectImage, and at that time, the placeholder is displayed. It causes flashing. So, I have to clear temporaryURL after media is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Let's use the derived mediaUrl string value for the condition instead of the media object.
|
|
||
| if ( ! media.id && media.url ) { | ||
| // This is a temporary blob image. | ||
| setTemporaryURL( media?.url ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| setTemporaryURL( media?.url ); | |
| setTemporaryURL( media.url ); |
Nit: There's no need for optional chaining here. See the condition above.
| return; | ||
| } | ||
|
|
||
| setTemporaryURL( media?.url ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for setting a temp URL here? The blob URL check above returns early, so the image will be uploaded when this line is reached.
This line causes the Site Logo to be displayed in the upload state when selecting an already uploaded image from the Media Library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, Must have added to test. forgot to remove it after.
| }; | ||
|
|
||
| const onRemoveLogo = () => { | ||
| setTemporaryURL(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also remove the call here. The " Reset " control uses the' onRemoveLogo' callback, which is only displayed after the image upload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it by removing this, It works fine.
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback, @sunil25393!
The changes test well for me ✅
What?
Resolves #59222.
Added loader while image is being uploaded in featured image block, Site logo block and cover block. Issue reported in #59222
Why?
Loading state which indicate that image is being uploaded was missing.
Testing Instructions
Screenshots or screencast