Image block: Show placeholder when uploading HEIC files#63643
Merged
Conversation
noisysocks
added
[Type] Enhancement
|
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. |
|
Size Change: +4.5 kB (+0.26%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
swissspidy
self-requested a review
July 17, 2024 06:39
andrewserong
left a comment
andrewserong
left a comment
Contributor
There was a problem hiding this comment.
Great idea!
Just left a comment about the placeholder state and whether it'd be worth reusing the existing placeholder if possible, to retain the transparent background and the height if the image was already in a placeholder state?
Comment on lines
+64
to
+69
| // Shown while image is being uploded but cannot be previewed. | ||
| .block-editor-image__skeleton { | ||
| aspect-ratio: 4 / 3; | ||
| background: $gray-100; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Would it be worth reusing the placeholder state for when an image is deselected? Currently that placeholder state uses a transparent background, and a different size, so with the gray color and 4/3, I notice a flash of gray and a different height during the upload. It's more noticeable if the image block exists on top of a darker background.
2024-07-17.16.48.27.mp4
It's very quick in the video since I'm testing Gutenberg without the core patch, so might be easy to miss 😄
Member
Author
There was a problem hiding this comment.
Deselected!! I thought we already had a placeholder state, but couldn't find it 😛 Good suggestion I'll do that.
Member
Author
There was a problem hiding this comment.
I've switched to using Placeholder.
I'm not a big fan of how the spinner looks on top of the diagonal line, though. It makes the spinner hard to see.
Mind you, this problem also exists on trunk. When there is a preview it can be hard to see the spinner depending on the image being uploaded.
Maybe we should put the spinner in a translucent box?
Or maybe we should denote that the image is uploading using a different technique e.g. pulsating the block's opacity?
Or maybe it's fine how it is?
What do you think @jasmussen?
Contributor
There was a problem hiding this comment.
What do you think @jasmussen?
I'm not sure if Joen is around at the moment, so I'll just give a soft ping to @WordPress/gutenberg-design for visibility 🙂
Member
Author
There was a problem hiding this comment.
Good plan. We can come back to this later, at any rate. HEIC is not very common and what's in this PR is better than what's in trunk.
Contributor
There was a problem hiding this comment.
I'd lean towards tweaking the overlay color/opacity a bit, so that the spinner remains visible, rather than adding another smaller 'surface' behind the spinner.
Another option could be a different loading animation in place of the spinner. The overlay could pulse between different opacity levels. That might require more holistic exploration though, so identify other applications.
Member
Author
There was a problem hiding this comment.
Here's what increasing the opacity of the placeholder from 0.1 to 0.2 looks like against a few different backgrounds:
That OK with you @jameskoster? If you want to play with it yourself an easy way to test is to make this change so that the placeholder and spinner will aways show for all image blocks:
diff --git a/packages/block-library/src/image/image.js b/packages/block-library/src/image/image.js
index 385a740e652..3417b1fc1a8 100644
--- a/packages/block-library/src/image/image.js
+++ b/packages/block-library/src/image/image.js
@@ -827,7 +827,7 @@ export default function Image( {
const isRounded = attributes.className?.includes( 'is-style-rounded' );
let img =
- temporaryURL && hasImageErrored ? (
+ true || ( temporaryURL && hasImageErrored ) ? (
// Show a placeholder during upload when the blob URL can't be loaded. This can
// happen when the user uploads a HEIC image in a browser that doesn't support them.
<PlaceholderAlso happy to punt this to later.
Contributor
There was a problem hiding this comment.
Thanks for that tip :)
Does the overlay background need to be currentColor? Since it's part of the UI I think it could use a UI color like $gray-900. Then with opacity something like .7 we get this:
This ensures the spinner is visible, and has similar visual weight to the dropzone treatment.
What do you think?
Member
Author
There was a problem hiding this comment.
I like it but I'm scared that I'm hiding a UI change that needs some design attention in an otherwise innocuous PR 😅
I'll revert f5e8ac8, merge this, and then open a new PR with your suggestion.
Member
|
Thanks for working on this 👍 This was on my todo list for a while as I need it for https://github.com/swissspidy/media-experiments (where this is also noticeable with HEIF images, but also MOV videos) |
Member
Author
Ideally when we have client side HEIC processing we'd still show the preview once it's been converted on the client. Future problem though 🙂 |
Member
That happens automatically already, the image is updated after processing :) |
andrewserong
approved these changes
Jul 19, 2024
andrewserong
left a comment
andrewserong
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the iterations here!
HEIC is not very common and what's in this PR is better than what's in trunk.
@jameskoster had some good suggestions in the thread, but if you wanted to land this as a first pass, I think you're right, it's an improvement on what's in trunk, and it doesn't affect the usual flow. So this LGTM! But do feel free to hold off if you wanted to iterate further.
Again, a lightning fast clip that shows the flicker of the placeholder state, as this is just testing Gutenberg without the core patch, while dragging an HEIC image onto an image block. But this placeholder state's looking pretty good to me:
This reverts commit f5e8ac8.
Member
Author
|
Thanks Andy. I'll revert f5e8ac8, merge this, and then open a new PR with @jameskoster's suggestion. Just so that design change gets a bit more attention. |
noisysocks
enabled auto-merge (squash)
July 24, 2024 04:38
Member
Author
|
Safe to ignore failing performance tests as they passed on 9cf0967 which we're now back at on the count of reverting. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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, why, how
In https://core.trac.wordpress.org/ticket/53645 we're adding support for HEIC images to Core. These will be automatically converted to a JPEG on the server after they're uploaded.
This PR improves the UX of uploading a HEIC image in a browser that doesn't support viewing HEIC images e.g. Chrome, Firefox.
Currently the temporary blob image that displays during upload can't be displayed and so the browser will display the default alt text during upload:
Kapture.2024-07-17.at.16.19.38.mp4
This PR detects when the image fails to load and during upload shows a placeholder
divinstead:Kapture.2024-07-17.at.16.21.59.mp4
In browsers that support HEIC aka Safari, you'll see a preview of the image while it uploads.
Testing instructions
It's a bit tricky, sorry. You'll have to set Gutenberg to use this branch and WordPress to use
adamsilverstein:add/heic-support(wordpress-develop#7034).One way to do that is to:
wordpress-developlocally andcdto itadamsilverstein:add/heic-supportnpm install; npx gruntcdback to your local Gutenberg repo.wp-env.override.jsonwith `{ "core": "/path/to/your/local/wordpress-develop/build" }npx wp-env startUnfortunately we can't use Playground because it doesn't support ImageMagick.
Once your testing environment is set up: