-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try: Media & Text max-width rule. #28922
Conversation
Size Change: -88 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
Nice @jasmussen - this worked well in my testing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Worked well in my local testing.
Thank you for the review Adam! I'm tempted to merge, but because the block is in wide usage, I'd love a few more eyes on it just as a sanity check. For context, I looked up the initial PR that added the But at the same time, the change feels totally resilient to me, testing with big and small images and video. Here's a friendly ping to @WordPress/gutenberg and if I don't hear anything in a couple of days, I'll and this! |
Might be good to get @joyously to verify, but I can't see any difference with this change. As I understood it, the intent of removing |
I revisited my original test post using WP 5.6.1 and the front end no longer shows the image overlapping text like in my original report. (tried with three different themes) Edit: Be sure to use a small portrait image to test. |
So it seems like the fix of the image overflowing on the frontend is no longer an issue. Is the issue then best described by the review in this PR, here? #28822 (review) — that is, small images are forced to fit the space? In that review I suggested 3 paths forward. I suppose this PR applies option 3. |
To me this looks like a good default CSS for this particular block. |
Yes, the issue is the small images being forced larger, but also matching editor and front end. |
I agree with pursuing option 2 for the reasons I explained here. If we don't change the current behaviour, then we should remove the Image size setting because it makes no sense to have it if we're effectively unable to resize the image: |
Good thoughts, all around.
I'm confused — the behavior currently being proposed would allow a small image to remain small, regardless of where you set the resize handle, right? The way the block works now, is that the image is sized from the left edge to the resize handle by default, and the height is automatic. And when the toggle — fill entire column — is flipped, it's both 100% width and 100% height. Both of those seem like deliberate intentions of this block, both of which will regress if we remove the behavior, to the point that you might as well have used 2 columns with an image in one of them. |
I mean we should remove the Image size select setting in the sidebar. Because unless we remove the |
Thank you for the clarification. |
I'm not sure how to move forward on this one, so going to close it for now. We can always revisit if there's value :) |
Can we reopen #28822 then? As that PR would fix #17787. I'm confused as to why it was closed in the first place. Or, if there really is strong opposition to removing |
These issues remain if we remove the width: #28822 (review). To an extent the conversation goes to the heart of the media & text block itself, because you could accomplish the layout using floats or columns instead. I see the key value being that the resize handle is a split between two types of content that scale to each. |
If we remove the width, we'll also have to set
In that case, let's close #17787 as |
Description
Make the media & text image rules more resilient to CSS bleed. Fixes #17787, props @Wingo5315, alternative to #28822.
How has this been tested?
Insert a Media & Text block, try adding a small image, a big image, or a video. All three should behave predictably in the editor, and the frontend should look identical.
Screenshots
If a theme has CSS to style the
img
tag (such as setting widths, min-widths or max-widths), those styles will affect the image inside media & text as well.Here's what could happen with a min-width:
Here's what could happen with a too-high max-width:
This PR adds explicit min and max-widths, but keeps the width, to make the behavior consistent.
Checklist: