Image: Try different resting state for placeholder, alternate version #43180
Conversation
|
Note that the intent of this PR was to not change anything about the Image Placeholder as it exists in trunk — black border, icon, title, help text etc. — purely make that hidden until you select. That way we can separate concerns and redesign that placeholder appearance separately. But as soon as I import the |
|
Size Change: +492 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Nice, this looks like a much more iterative approach that solves a large part of the original issue :) |
|
This PR would benefit from #43228 landing first. |
jasmussen
force-pushed
the
try/image-placeholder-improvements-alt
branch
from
f414870
to
9b8898d
Compare
|
This PR has been rebased, and currently looks like this: A few tasks remain:
2 and 3 are likely followups that are best handled after #42847 has landed, and are followups regardless. But 2 brings with it some interesting questions, that may even carry an answer to 1. Once the placeholder can inherit radius, it's going to affect the content inside, potentially even crop or reflow it. Here's an example of a pill-shaped image:
If we're super happy about the idea of keeping the selected state identical to the trunk appearance of the placeholder, we could do this instead:
Would love thoughts on that, but am going to try a thing in the mean time. |
|
Alright, just to keep things flowing, I pushed a change to make it look like this:
It's minimal, it could use more love, but it does include the important buttons from the base Image block. At the same time, it scales to being potentially cropped by a wide border radius. Is this something we could see ourselves landing? |
|
This looks great. One tiny thing I noticed when I resized the window is that the Media Library button (which is not inside a div like the other two buttons) tries to fill the container. Maybe this is a non-issue because right now it looks more or less ok, but if the container was larger, it would probably look strange. Screen.Recording.2022-08-16.at.11.40.17.mov |
|
The failing test is this one: The final thing to do to land this is to either decide that this look is okay for the first version, or whether we need to restore the site icon/title/description and go this route (screenshot is missing title/description). I'd lean towards the latter, not because it looks better, but because then it doesn't touch the design of the default placeholders at all, letting us do that holistically and separately. Would appreciate help with that one too. |
|
Fixes #41142. |
jasmussen
force-pushed
the
try/image-placeholder-improvements-alt
branch
from
fd1f067
to
c7f4f53
Compare
|
Alright, with the help of @MaggieCabrera I've brought this branch to a state where I think we can merge the first version. Outside of a test fix, recent changes bring the placeholder back to look identical to what's shipping:
It isn't the best look, and the classic placeholder hasn't aged too well. But in not touching it, the experience remains unchanged to what's shipping, except for the unselected state. And there will remain still a great opportunity to revisit the placeholder experience here holistically, to reduce and simplify. But this PR will be a step on the way, but not the end goal. It will still be able to inherit border radii, once we can replicate something like #42847 for this PR:
Let me know your thoughts! |
|
All green checks 😱 For folk subscribed, please feel free to give this another thorough test per the last changes. We're now in a good place in terms of landing this for 6.1, so if anything pops up we should be able to fix it! Thanks again everyone. |
jasmussen
force-pushed
the
try/image-placeholder-improvements-alt
branch
from
8a728f1
to
814fa5f
Compare
|
Alright, we've tried this style for Featured Image. And even though there's more work to be done, this on-select/focus appearance change feels like a careful start. There's still time before the code freeze so I'll merge early in case we need to get some feedback from trunk and revisit. |
|
Unfortunately this has made it so that errors aren't shown when uploading an image via the image block. To reproduce:
Expected result: An error notice is shown. To fix this we need to pass |
|
Apologies for the regression! As far as fixes, can we show a snackbar instead? The notices inside the placeholder can never scale to the size of a default 120x120px Site Logo, so it's high time we removed the in-placeholder notices. |
|
I believe the regression is only with the Image block. Both Featured Post Image and Site Logo were already using snackbars. In general, I think MediaPlaceholder is due to some clean/refactoring 😅 |
|
I would be happy to help in any way I can to making the snackbar happen for Image. |
|
I can push the required changes later today 👍 |
|
🙏 |
What?
Alternative to #43143 with the intent to make smaller changes. This one keeps the more classic setup state appearance onselect:
Why?
With Featured Image, Site Logo and soon Image sharing a variety of implementations of this appearance, and further discussion ongoing in #42847 (comment) as to how to visually style that state, I wanted to see if we could land a smaller interim step.
Testing Instructions
Insert an image block and observe the new resting state.