-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix Core Columns butting up against viewport on smaller screens #20589
Conversation
…n smaller viewports
… smaller viewports
Size Change: +114 B (0%) Total Size: 865 kB
ℹ️ View Unchanged
|
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.
I'm not sure if I tested correctly due to my screenshots being different from yours. Some more eyes on it would be good. If mine ends up not being right I'll remove the change request. Otherwise it's just two tiny comment spelling corrections 👍
// on larger viewports where width handles this. Specificity overide required to | ||
// overide generic `.entry-content *` rule |
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.
overide
should be override
// on larger viewports where width handles this. Specificity overide required to | ||
// overide generic `.entry-content *` rule |
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.
overide should be override
@include break-xlarge() { | ||
padding-left: 0; | ||
padding-right: 0; | ||
} |
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.
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.
Thank you! This improves things, but I don't think it's the right fix, and I think the regression was potentially due to #17877 — sorry about that.
Specifically if I inspect, the margins look correct a great deal of the way down:
So what makes the text butt out like that?
This rule:
That margin-left: -14px (probably -$block-padding in the code) looks like a remnant of when it was necessary to compensate for the left/right margins that have been refactored.
And then I git pulled. The above is no longer accurate it seems. So, master is different now, possibly due to improvements in #20597.
Now the text only buts out if the columns block is full-wide, and then honestly it seems expected? Because it does so on desktop breakpoints also:
To phrase it differently, there was definitely an issue, but the issue has either changed or even become obsolete overnight. Let me know what you think of master.
@aduth I'm afraid I don't. However, there's a lot of changes to widths and spacing in those PRs so it could be they caused this. I might try testing altering a few of those properties (esp. I would also say that's it is extremely easy to introduce this kind of regression given the number of variants (Editor, Frontend, different Themes...etc). Not sure how we solve or guard against this... |
@jasmussen Thanks for this. I've pulled I'm still concerned this issue could be prevalent on certain Themes so I'll dig into that, but for now I'm happy to close this out. 🎉 |
Yep. There was a dev-note attached to the margin change, but it is unfortunate that it had to happen. The silver-lining is, however, that the margin math is vastly simpler now. |
On smaller screens, the Core Columns block butts up against the edges of the viewport (unless it has a background set) on both Editor and Frontend.
This PR aims to fix this by introducing left/right spacing around the Block on smaller viewports. This has to be handled differently between Editor and Frontend/Theme due to the way background padding is applied.
See screenshots below.
My feeling is that without this fix Core Columns looks broken on Themes that don't have specific CSS to add space on left/right. I feel that Core should try to avoid things looking "broken" by default.
I would, however, welcome some guidance and direction from those more experienced with the wider Themes ecosystem.
Also relates to Automattic/wp-calypso#39804.
How has this been tested?
Environment
This has been tested using the Gutenberg Starter Theme because it enqueues no Theme specific styles so we see the raw Block styles.
Note that on Twenty Twenty this fix is not needed because the
block-editor-block-list__layout
rules add space on left/right by default. However this is not the case for some Themes.Instructions
Switch to "Code View" and paste in the following test Block definitions:
master
- resize screen to a small viewportDo the same on both Editor and when viewing post on Frontend.
Screenshots
Before
After
Types of changes
Bug fix (non-breaking change which fixes an issue).
Could possibly break some Themes.
Checklist: