-
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
RSS Block: Add border-box CSS #27767
Conversation
Size Change: -23 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
This is a cool PR, but I'm seeing some weirdness that is probably unrelated to this PR, but also some that is. Screenshots after: Frontend after: Before is the same, actually: It appears there's a ton of padding on the RSS feed block caused by this:
that appears to affect both the placeholder state, and the rendered state, but only in the editor. Furthermore, in the editor the box-sizing property appears overridden by this: I appreciate trying to keep the PR small, and I think it would be fine to land this. But since you're here — are you seeing the same issues, and should we look to fix them at the same time? |
9b327cf
to
6f1c0f8
Compare
You're right, I removed the padding and added an |
This is cool: But the need for an This seems to accomplish the same:
👆 it will need a linter rule opt-out because the linter will say something about duplicate rules, I can't recall which rule. But it would elevate the specificity without going nuclear. What do you think? |
Yeah I thought about that option but I thought that generally people would be more familiar with the Another option could be to modify the mixin so it doesn't target the rss block - It feels like we need a way to gradually move towards setting border-box on everything, but we've quite aggressively prevented that in the past, so it's going to be a long road! |
The problem with !important is that it's extremely hard to override, whereas the other approach adds just a little specificity, so I personally much prefer the other approach. But in order to not be blocked on this, I'm happy to defer to a sanity check from @kjellr or @MaggieCabrera. |
box-sizing: border-box !important; // The important is needed because of the reset mixin on the parent: https://github.com/WordPress/gutenberg/blob/a250e9e5fe00dd5195624f96a3d924e7078951c3/packages/edit-post/src/style.scss#L54 | ||
// This needs extra specificity due to the reset mixin on the parent: https://github.com/WordPress/gutenberg/blob/a250e9e5fe00dd5195624f96a3d924e7078951c3/packages/edit-post/src/style.scss#L54 | ||
&.wp-block-rss { | ||
box-sizing: border-box; |
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 prefer this much more than !important. I wonder why the theme css is not loaded after the editor's, wouldn't that solve all these kinds of issues?
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.
Yeah, I prefer the double-selector approach here too. 👍
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.
It seems the extra specificity is only needed in the frontend, so I wonder whether we should keep "style" pure and add the specificity in "editor.scss". Not a big deal though.
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.
IMO the benefits of having the same code in the editor and frontend are greater so I'd prefer to keep it like this.
2c48321
to
bafe080
Compare
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.
Let's get this in. Thanks Ben.
bafe080
to
13c0396
Compare
Description
I think we should add
box-sizing: border-box
to the RSS block. The block has a fairly large amount of padding. If we set the block to be full width withwidth: 100%
then we'll get horizontal scrollbars on the page because of the padding. This CSS will fix that.How has this been tested?
In TT1 Blocks
Types of changes
Breaking change - this could potentially break existing implementations, although I think it is more likely to fix them.
Checklist: