Try: Overlay caption w. text-shadow. #63471
Conversation
jasmussen
added
the
[Block] Gallery
|
Size Change: +1.68 kB (+0.09%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Nice! This may be perhaps out of scope but has the option of a left/right aligned overlay caption been considered? |
|
I'd love for a separate "Caption" element to be stylable, just like buttons, forms, inputs, etc. Right now that's missing, and I'd like to see that, this would also allow font size changes, alignments, color changes, etc. |
jasmussen
force-pushed
the
try/overlay-caption-polish
branch
from
c59cdce
to
ffedb1e
Compare
|
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. |
|
Marking this one ready for review. The combination of background blur, a refined gradient, and the text shadow keeps contrast the same as before in most contexts, but improves it when many lines of overlay caption text is used. It's also close enough to the existing style that it is firmly in improvement territory, which is important given this is already shipping and in use. Would appreciate your reviews! |
There was a problem hiding this comment.
This is a great improvement. However, what I'm seeing seems a little different for some reason.
There's space on the left and right of the scrim. After some research, it seems like this is being affected by scrollbar-gutter: stable both-edges; which is part of the style of the custom-scrollbars-on-hover mixin. This issue only appears in certain environments (Windows OS, Chrome):
| Chrome | Firefox |
|---|---|
I also believe that we need to address two issues.
If the caption is scrollable, the bottom position of the scrim will move:
This style needs to be removed when the Image block is in the "Rounded" style:
|
Just wanted to note that I really appreciate your excellent feedback, and will address it! Other things got in the way first, but the review hasn't been forgotten. Thank you 🙏 |
jasmussen
force-pushed
the
try/overlay-caption-polish
branch
from
da1c8cf
to
5f3d3a9
Compare
|
Btw |
|
Took a stab. This was harder to address than I thought it would've been, so it needed kind of a fresh approach. What I've done now, and I believe this actually addresses all the items you brought up (though good to test again), is this:
The combination of these two, I feel, still captures the spirit of the effort, but should work better:
|
There was a problem hiding this comment.
Thanks for the update! It looks like it's almost complete.
After testing it again, I noticed the following.
Personally, I feel that blur(4px) has a slightly large impact depending on the content of the image. What do you think about 2px?
4px blur:
2px blur:
Another thing I noticed is that the caption height is not constrained. Should we apply max-height: 40% if we want it to match the scrim?
| // Create a background blur layer. | ||
| &:has(figcaption)::before { | ||
| content: ""; | ||
| z-index: 0; |
There was a problem hiding this comment.
There doesn't seem to be any visual difference without z-index: 0, but is it necessary?
|
|
||
| // Place the caption at the bottom. | ||
| figcaption { | ||
| z-index: 1; |
There was a problem hiding this comment.
There doesn't seem to be any visual difference without z-index: 1, but is it necessary?
There was a problem hiding this comment.
Good catch on both z-indexes. They were briefly necessary in one of the versions I was testing, but in this final version they are not.
| color: $white; | ||
| text-shadow: 0 0 1.5px rgba($color: $black, $alpha: 1); |
There was a problem hiding this comment.
| text-shadow: 0 0 1.5px rgba($color: $black, $alpha: 1); | |
| text-shadow: 0 0 1.5px $black; |
Since rgba($color: $black, $alpha: 1) is the same as black, I think it can be made simpler.
| box-sizing: border-box; | ||
| @include custom-scrollbars-on-hover(transparent, rgba($white, 0.8)); | ||
|
|
||
| // Dark gradient scrim. | ||
| background: linear-gradient(0deg, rgba($color: $black, $alpha: 0.4) 0%, rgba($color: $black, $alpha: 0) 100%, transparent); |
There was a problem hiding this comment.
| background: linear-gradient(0deg, rgba($color: $black, $alpha: 0.4) 0%, rgba($color: $black, $alpha: 0) 100%, transparent); | |
| background: linear-gradient(0deg, rgba($color: $black, $alpha: 0.4) 0%, transparent 100%); |
The gradient style should remain the same.
|
Great feedback as always, thank you.
Can we meet at 3px?
2px feels so small to me that it might as well not be there:
And by that I mean, if we're going to do it, it should be perceptible, otherwise best to not do it. I'll go through your other pieces one by one now! |
I intentionally let it be unconstrained, what I found as I tinkered was that when constrained like we do now, the scrollbar behaves somewhat curiously since it only scrolls the caption. That also means that the caption itself can be cropped, which can look awkward, like so:
Finally for really small images with captions, if we constrain the height, there might not be space to show anything at all. That said, I'm happy to restore it to keep this PR smaller: it's mainly about elevating the quality and clarity of the scrim, than changing the behavior, so no strong opinion from my end. Let me know if I should restore it! |
There was a problem hiding this comment.
LGTM! I left just a few suggestions at the end, but it all seems to work fine.
Finally for really small images with captions, if we constrain the height, there might not be space to show anything at all.
I had overlooked this scenario. It certainly seems like it would be better to leave the height unconstrained.
I was concerned that the lightbox toggle button would become unclickable when the caption reached 100% height, but that doesn't seem to be an issue:
| overflow: auto; | ||
| padding: 0 8px 8px; | ||
| position: absolute; | ||
| padding: 1em; |
There was a problem hiding this comment.
This is a non-blocker suggestion.
Because scrollbar-gutter: stable both-edges has been applied to the figcaption, the padding on the left and right looks wider, we may want to make it a little smaller.
Below is what it looks like on my environment, but how does it look on yours?
padding: 1em:
padding: 1em 0.5em:
There was a problem hiding this comment.
I don't disagree, though since this PR already touches a fair bit of CSS, I wonder if it's not best to do this separately?
Thank you again for the thorough reviews!
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
Just checking, is this good to merge? |
|
I'm going to merge, but happy to follow up! |
|
Sorry for the late reply. Of course, my suggestion regarding padding could be tried in another PR 👍 |
Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: beafialho <beafialho@git.wordpress.org> Co-authored-by: paaljoachim <paaljoachim@git.wordpress.org>
|
Hei Joen. I have a comment here... In Gutenberg plugin 19.1 Gallery Image captions now look like this:
The artist web site I am creating the artist does not want the caption on the images inside galleries so I moved these below with the following code. The result looks like this: I should have tested this PR before it was merged with the setup I have for the web site, but I did not think about it. I am now having a difficult time using an important to remove the text shadow in the custom code. I added this text shadow code which seemed to work out alright with the rest of the code. So that it now becomes like so: |
|
One more thing. I noticed the bottom part of the Gallery images are blurred where the caption was earlier. Gutenberg 19.1 active. Gutenberg 19.1 not active. |
|
Can you try adding this CSS to your custom override? That said, it's not clear to me this is necessarily an issue with the CSS as written, since it's meant to be an overlay. One thing we found out is that, although there's no official support for captions below the image in a gallery, there's a little trick. If you apply a border-radius to the image in the gallery, the caption gets pushed below: CC: @aaronrobertshaw I noticed that this trick to move the caption below the image only works when the radius is applied locally. If you apply a border radius to the Image block in global styles, it does not change the galleries. Could it? Should it? It seems like if the behavior is in place locally, it should be in place globally as well, no? |
|
Thanks for the ping 👍
That's my understanding as well. It was introduced via #27869 into the overall refactor of the Gallery block to use inner image blocks. The way it works is the image block will add a
I believe this was due to there being nothing for the block's styles to hook onto to apply the CSS to reposition the caption conditionally.
Could it, or should it? I'm not sure on either count. Whether captions should be overlayed or not seems to me like it should be a block setting or perhaps a block style variation. Better yet, I'm still a fan of a dedicated Caption block which could encapsulate this sort of behaviour and styling. Since the Gallery block refactor, support for custom CSS in Global Styles was added. That could be leveraged to reposition a caption if desired. We now have other blocks adopting border-radius support that can result in clipped content e.g. Group, Column etc. They are left up to the user or theme author to handle whether that's via extra padding, custom CSS etc. From memory, the caption repositioning was a compromise during the Gallery refactoring between the pre-existing overlaid captions and the capabilities of the image block. To maintain backward compatibility now, we might be stuck with block instances repositioning the caption. To then bring the Global Styles side of things in line with the block instance behaviour, all I can think of are hacks to the Global Styles border panel for a single block type (bit of a non-starter), or defining a Gallery block style variation including custom CSS to handle the repositioning. |
Thank you Joen! The ideal would be go with....
As @aaronrobertshaw mentioned. As it would hopefully make it openly possible for a user to choose the location (and have editing tools) of the caption in relation to the image in an Image block and a Gallery block. A summery: |
What?
Try to make progress on #8030 and #56587 by improving the caption style.
The current dark scrim is a very dark gradient to aid legibility for the white text on light backgrounds, however the sharpness of the gradient itself makes the legibility sub-optimal:
This PR does a few things to keep the spirit of the same style, but improves it by:
Testing Instructions
Insert galleries with overlay captions.