Image: Make Placeholder white when there is a <Spinner /> on top #63885
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
noisysocks
added
[Type] Enhancement
noisysocks
requested review from
ajitbohra,
ellatrix and
artemiomorales
as code owners
|
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. |
noisysocks
force-pushed
the
update/placeholder-design
branch
from
1453f90
to
057a5de
Compare
|
Size Change: +4.37 kB (+0.25%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
I flip-flopped back to a white overlay, the contrast against the gray spinner track is less harsh. Also pushed a couple other style tweaks, and removed a margin that was making the spinner slightly off-center. I'm a bit confused by the behavior though. I'm seeing the overlay without a spinner when an image hasn't been uploaded. Shouldn't both the overlay and the spinner become visible whilst the image is being uploaded? |
|
The overlay will show when you have not picked an image and you deselect the block. A faded out image + spinner will show while an image is being uploaded. The overlay + spinner will show while an image that isn't supported by your browser (e.g. HEIC) is being uploded. Does that make sense? Thanks for pushing. The diagonal line isn't appearing on the overlay anymore because of the 
I imagine that's a mistake? I'll remove the 
Here's how everything looks together: Kapture.2024-07-25.at.11.39.29.mp4And here's how the placeholder looks like when it's on top of a background image: 
I like it! Add an approving review if you're happy 🙂 |
Hmm, I don't understand why we're showing an overlay when the block is deselected. What purpose is that serving? I'd expect to see the standard placeholder with no overlay, this one: 
My understanding was that the overlay's purpose was to ensure the spinner was visible? The Here's what I'd expect: 
Does that make sense? |
|
Sorry, think there's some confusion. What do you mean by overlay? In my comment above I used overlay as a synonym for the When an image that the browser can render (JPEG) is being uploaded, we show that image with the When an image that the browser can't render (HEIC) is being uploaded, we show a When an Image block with no image selected is deselected, we show a When So I think things are currently working as you expect? There's no overlay on top of the |
|
Ah I see, thanks for clarifying. Sorry about the confusion, that's my fault. I didn't realise the overlay and the placeholder were the same element. The part that doesn't seem to be working as expected is that the white background should only applied when the spinner is visible. Currently it seems to be applied regardless. If it's not trivial to do that, then it's probably okay to just leave things as they are (but include the margin fix). |
👍 you got it, done in b54c323. |
noisysocks
changed the title
|
Bingo, here's what I see: 
The only other minor tweak I'd suggest (if it's possible) would be to reduce the opacity of the diagonal stroke, it's obscuring the spinner a little. Otherwise this seems good. |
jameskoster
approved these changes
Jul 30, 2024
Done 👍 |
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?
Updates the design of the
Placeholdercomponent when passed thehasIllustrationflag.Why?
Moving to a darker design based on the UI colour so that you can more easily see any
Spinnerplaced on top. See #63643 (comment).How?
I did what @jameskoster suggested in #63643 (comment) 😀
I also removed the
placeholder-stylemixin because it isn't really doing anything in most of the places it's used. The mixin adds&::beforestyling and we're often including it into a&::beforeor&::afterwhich means there is no effect.As far as I can tell this styling is only used in
<Placeholder hasIllustration>which is the media blocks when not selected or when uploading.Testing Instructions
If you make these changes locally you can force the
Spinnerto always appear:Then:
Spinneron top.Screenshots or screencast