Images: Try moving responsive rule to common.scss. #38399
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
added
[Block] Image
|
Size Change: +298 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
kjellr
reviewed
Feb 1, 2022
kjellr
approved these changes
Feb 1, 2022
Co-authored-by: Kjell Reigstad <kjell.reigstad@automattic.com>
MaggieCabrera
approved these changes
Feb 1, 2022
|
Excellent, thanks all for testing. I'm going to let this sit overnight and merge in the morning unless someone comments their concern. |
|
Cool, let's try this, it's a small change and easy to revert if we need to. |
kjellr
added
the
Backport to WP Minor Release
|
Marking this one for backport since it solves a glaring issue with older content. |
Mamaduka
removed
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Feb 16, 2022
* Images: Try moving responsive rule to common.scss. * Update packages/block-library/src/common.scss Co-authored-by: Kjell Reigstad <kjell.reigstad@automattic.com>
|
gutenberg/packages/block-library/src/common.scss Lines 129 to 132 in f111f0c Is this rule meant to be making it to the frontend globally? |
|
Yes, with low specificity. Is it causing trouble for you? |
|
Yes unfortunately. Because it applies to all images, it is also applied to images on the site that don't come directly from the block or classic editor. In my specific case I had draggable slider driven by images that do not have explicit width definitions that broke. If the goal is to achieve the same responsive treatment to images that came out of the original classic editor, could this be achieved with a more specific rule? Or because we don't know what div that the post content is getting dumped in, there is no way to make it more specific apart from html/body. Sidenote: I've never seen a :where in SASS before and have been unable to find documentation on it, could you please refer me? |
|
@MaggieCabrera do you have any ideas on how we might scope this rule to be content specific? @carlaiau |
Yeah
Thanks very much! |
As long as it covers images that can be inserted via the editor, that would be fine (image blocks, images inserted in the classic editor, etc). Any other kind I think it would be fine to be handled by the theme or a plugin instead. |
|
I just saw a similar issue report - #39021. |
This is happening to me as well, which unfortunately means I would have to check which images in my themes don't have How about a more specific rule that selects images by the dynamic Don't know about the css performance implications of using a regex css selector in huge sites though, but I think it is a better solution to avoid the problems of a more general rule. Maybe we could keep the What do you think? @jasmussen |
|
adding the regex to the :where could be a go-er! |
|
Just confirming here that this change broke our 15+ year old site with spacer gifs and such 🙂 |
This was referenced Feb 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #38384. We have CSS in place to provide a default responsive behavior for Image blocks (
max-width: 100%; height: auto;), but this doesn't affect images inserted in the classic block. Before:This PR moves that responsive baseline to the
common.scssfile so it becomes boilerplate CSS rather than CSS specifically for the Image block. After:I'm unsure whether this is good practice. There's an argument to be made that the responsive behavior is a feature of the image block, and that when you're inserting a non-block image, it can be a sign that you want the freeform behavior. Also, the less boilerplate CSS, arguably, the better.
Nevertheless this does fix the issue, even if this PR needs testing across every single element in the canvas that uses Images, if we want to land it.
Testing Instructions
Insert test content, such as from this comment, and observe as in the screenshot above that the classic image is bounded by the main column.
Checklist:
*.native.jsfiles for terms that need renaming or removal).