Post Featured Image: Add border support applied to inner img #42847
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
This should be pretty close to reviewable, I've run out of time today to test fully but will do that later this week. |
|
Size Change: +1.65 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Nice! This is working well for me. Would it be possible to also apply the styles to the placeholder you find in the template editor when a featured image isn't set, or when no context is passed?
|
It should be possible. I'll take a run at it later this week. |
I've explored applying borders to the Post Featured Image's placeholder and run into a few issues that make it a bit of a clunky fit. Theme.json / Global StylesTo display any borders set via theme.json or global styles on the placeholder, we'd need to add an editor-specific selector to the block.json config, which would also result in that selector being included in the CSS generated for the frontend. This is minor, but it might add up if we intend to do this for every image-related block or any block supporting borders but using a placeholder. Block specific bordersFor borders set via block support, we can manually apply these to the placeholder. The downside is that the application of the border can really detract from it appearing as a placeholder. To achieve this cleanly, we'd need to update the way Another UX issue here is the default placeholder styles define Example of borders detracting from placeholderScreen.Recording.2022-08-04.at.3.15.12.pm.mp4Image BlockThe Image block does not apply borders to its placeholder state. I appreciate that the use cases here are a little different but given the similarities between the blocks, should they be consistent? Before investing too much time here, I'd like to double-check how desirable applying borders to the placeholder actually is. I'd also be happy to continue exploring this in a follow-up. |
I think it's pretty important. Not so much in the post editor, but in the template editor – where this block is going to see more use – definitely. If I'm designing my Single template and would like to add a border to the featured image, I'm going to want to see a preview of that border on the canvas. I don't find the border detracting from the placeholder to be a big deal. The diagonal dashed line is the key visual indicator so as long as that's partially visible most of the time then we should be ok.
It's probably not out of the question to update the placeholder design a bit to better accommodate the border controls. Since this could be a bit tricky to get right, a follow-up may make more sense. |
|
@jameskoster I've updated this PR to apply the border styles to the block's placeholder. It's mostly there except for a small issue around the border radius for the placeholder's SVG. If I make the SVG inherit the placeholder's border radius (current state of this PR), depending upon that value and the width of the border, we get a small gap between the placeholder border and the SVG's.
An alternative was to clip the SVG by setting |
|
@aaronrobertshaw could we remove the placeholder border together if one is added via the border control? 🤔 |
So I'm on the same page, the "placeholder border" to me is the blue border in the screenshot in my earlier comment. The dotted border is on the placeholder's inner SVG. Am I correct in assuming it is that dotted border you are asking if we can remove if a border is set? We could if the border is added at the individual block level however that is more problematic when the borders are added via Global Styles or theme.json. In those cases, the border styles are applied via a generated stylesheet. In the block editor, for example, we don't have access to this data short of getting computed styles. |
Yes.
That sounds like it won't be possible? Making the radii match would be acceptable I think (in a follow-up like you mentioned). Alternatively we could explore different design approaches to the placeholder that do not use a border. |
|
This is damn sweet. Let's for sure land this for 6.1. Here's a status with the border radius issue as discussed above: I tried a trick that I think solves it:
What do you think? It feels solid to me. This is the trick: You can paste that on line 108 in editor.scss. Let me know what you think. |
|
Inspired by excellent work here, I've started #43143 to do the same for the Image block. I would be grateful for some help in porting over some of the inheriting features from this PR. The opportunity to refactor the placeholder component, or just componentize the dashed-line variant, is also increasingly becoming clear. Probably doesn't have to happen now, but it'd be a nice optimization at some point now that we're using this style across both Featured Image, Image, and Site Logo. |
|
Thanks for the enthusiastic reception as well as taking a look at this one @jasmussen, appreciate it 👍
Unfortunately, there are a few issues with the suggested CSS. Which is actually a subset of what I had in mind with my #42847 (comment).
The main issue is that when border styles are generated via theme.json or global styles, we'll end up with something like the following in the generated stylesheet: .wp-block-image img,
.wp-block-image .block-editor-media-placeholder {
border-style: solid;
border-color: #00000;
border-width: 5px;
}The border isn't applied by a custom CSS class or style attribute so I'm unaware of any means to conditionally style the inner SVG based upon that. A primary use case raised earlier was setting a global style so featured image blocks within the query block in the site editor would display with a border. In such a scenario we'd still get the misaligned borders. The second, easily solvable issue, with the proposed CSS is that it only covers flat borders as opposed to those applied to individual sides e.g. top and bottom borders only etc ( I'm hoping to have a little time to experiment further with the placeholder today. One thing I'll try is to perhaps absolutely position the SVG and give it the same border, to essentially overlay it on the placeholder wrapper. |
|
Ah, thanks for the clarity, I had a feeling. Another option is to draw the dashed border on the placeholder component instead of the SVG, so it will naturally get overridden. The main problem there is that the border is intentionally set to be |
|
Alright, I tinkered with this enough to get a headache. Trying clever inheritance tricks, I got really close while maintaining the semi-transparent currentColor stroke: But unfortunately, I couldn't thread the needle. There was always some tradeoff, and the complexity was getting out there. So I tried a different approach: ditching the opacity entirely:
That's with the following code diff: Or, you can manually integrate it with this chunk: — but you have to remove the So the question is: is that text-color dark outline good enough? I think yes, but I'd love input from @WordPress/gutenberg-design. Keep in mind, this should also affect Site Logo and soon Image in #43143. What do you think? |
Do you mean on the 'No color, but width and radius defined' config? That's a tricky one. It feels a bit strange to see a color applied when one isn't explicitly set. But I appreciate that's an accurate representation of how Perhaps as a follow-up we can consider making the Border color control behave like the spec, IE the default border color is inherited from the text color of the selected element (when we're able to detect it 😅). |
|
There's some nuance. Right now the placeholder is drawn using
What I'm saying is that we can have either of these two, but not both at the same time:
Is 1 fine? I suppose that's the main question. |
|
I think it's fine. In fact, I think I prefer it 😆 |
|
Cool. We should probably try it. The thing is, we can always come back to these (especially if we can clean up the code and componentize it a bit) and try a different style if we like. |
|
I've gone ahead and created #43228 which deduplicates and simplifies the code for these dashed placeholders. Part of that PR is also a fix that should enable border inheritance in this PR. Edit: now merged. This PR should hopefully just work if rebased. Let me know if you need any help! |
aaronrobertshaw
force-pushed
the
update/featured-image-block-with-border-support
branch
from
e471234
to
8c31798
Compare
|
This PR has been rebased. The video below demos the current state of borders on the Post Featured Image block. There are still several issues with applying borders to the block's placeholder. The end of the video includes a demonstration of one that occurs when opposite sides get differing width borders. Another is when the block sets a zero-width border, a It feels to me that the styling of the placeholder has regressed now rather than been improved after recent efforts to inherit the border on the SVG. I'd like to propose that we give this a final review to merge and address the placeholder problems in a separate follow-up. Screen.Recording.2022-08-18.at.2.40.35.pm.mp4 |
|
I have not yet tested it, but from looking at the video Aaron this looks really nice! I want it! |
|
This is great, my GIF also shows border and radius working well in the setup state:
I'll take a quick look and if I think up a fix I'll push! Hope that's okay.
I think this is acceptable. It's first and foremost in edgecase territory, and secondly it's technically accurate. |
|
@aaronrobertshaw I pushed a fix for the inner color: What do you think? If you're cool with it, looks like this one needs updated snapshots. |
There was a problem hiding this comment.
Experience wise, this is a step forward for letting placeholders inherit the styles of their surroundings. Across the board this effort can improve patterns and placeholders, so things like dimensions and even duotone can be visualized and preset, without media being added first. In that line, this is a solid win.
I would appreciate a quick code sanity check, but otherwise this one seems good to try!
I agree. Still, it might be good to (separate to this PR) explore adding a background to the placeholder so that it is still visible when placed on a noisy background, or when placeholders overlap, e.g. putting featured image inside a cover:
|
Ack, yes indeed. |
|
Thanks for tweaking the placeholder styles @jasmussen 👍
It might just be me, but I still think the placeholder with borders has problems however, if we remove the styles inheriting The following videos show what I see with and without the abovementioned styles.
Given the previous commit adjusting these styles, I'll hold off merging this until we decide whether to strip them or not. My vote is if they don't impact other block's placeholders, we should remove them. Any other issues with the placeholder, like applying a background color are out of scope for this PR and will be addressed via follow-ups. |
This makes the diagonal line appear more consistently.
|
Excellent sleuthing. As far as I can tell, the major difference is the positioning of the diagonal, which is off when those styles are inherited: It is not off when they are gone:
I could swear I added those for a reason, but testing with a rebase the new Image, as well as Site Logo and various combinations of borders and no borders, this change seems solid! In other words, they don't seem needed. Definitely remove those, then land this. Thank you! 🙏 (Besides, if they turn out to become necessary again, we'll just add them back.) |
|
Thanks for the continued testing @jasmussen 👍 I've pushed a commit to remove those styles as discussed. Once the e2es are green, I'll get this merged. |
aaronrobertshaw
deleted the
update/featured-image-block-with-border-support
branch
|
💥 |
|
I'm debugging in a PR of mine with Query Loop work and I have noticed these: Does it ring any bell? It might be me though... still looking 😄 --edit It seems to be on trunk by this PR.. |
Related:
What?
Adopts border block support for the Post Featured Image block and applies those classes/styles to the inner
imgelement.Why?
This is a step towards reducing the gap between what you can control with the Image block and the Post Featured Image block. In terms of the bigger picture, it is part of providing more consistent design tools across blocks.
How?
imginstead of the block wrapperTesting Instructions
Example snippet
Screenshots or screencast
Post Editor - With Image
Screen.Recording.2022-08-01.at.7.52.24.pm.mp4
Post Editor - Placeholders
Screen.Recording.2022-08-08.at.2.52.07.pm.mp4
Site / Template Editor
Screen.Recording.2022-08-08.at.2.41.35.pm.mp4