-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Image: Make Placeholder white when there is a <Spinner /> on top #63885
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. |
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? |
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. |
Done 👍 |
What?
Updates the design of the
Placeholder
component when passed thehasIllustration
flag.Why?
Moving to a darker design based on the UI colour so that you can more easily see any
Spinner
placed on top. See #63643 (comment).How?
I did what @jameskoster suggested in #63643 (comment) 😀
I also removed the
placeholder-style
mixin because it isn't really doing anything in most of the places it's used. The mixin adds&::before
styling and we're often including it into a&::before
or&::after
which 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
Spinner
to always appear:Then:
Spinner
on top.Screenshots or screencast