Social Links Block: Prevent Theme Styles Distorting Size#56301
Social Links Block: Prevent Theme Styles Distorting Size#56301apeatling merged 3 commits intoWordPress:trunkfrom
Conversation
Would be appreciated if somebody could add a label to this one! It'd be nice to see this looked at since there's been some more reports of this issue. cc @jorgefilipecosta, @Mamaduka as most recent contributors to this block - sorry for the ping! |
apeatling
added
Needs Design Feedback
apeatling
left a comment
apeatling
left a comment
There was a problem hiding this comment.
Confirmed this fixes the issues listed. It would be good to get some feedback from @WordPress/gutenberg-design for any concerns about conflicts when setting margin/padding like this.
| } | ||
| } | ||
|
|
||
| &:after { |
There was a problem hiding this comment.
I have a feeling this is the main part of the fix, but I'm also not sure. Can you speak a bit more about what this does? In the editor we do use some pseudo elements to display focus styles, just want to make sure we don't regress that.
There was a problem hiding this comment.
Hey @jasmussen - no, the padding and margin is the main part of the fix which covers most themes. However, at least two themes which I tested also set an :after in the widget area (eg. see Apostrophe 2 in Automattic/themes#7062).
I feel that more than enough themes are affected to justify the padding and margin styles in Core, but I don’t mind particularly either way on this bit. However, since we already have a selector, I suspect that it’s probably easier for users and developers than hoping that themes update their styling for this block.
I hope that makes sense, and thanks for the review! :)
There was a problem hiding this comment.
The fix is definitely valid, my question is mostly around this pseudo element. Do you know where this code is coming from?
If it's reproducible in core on more than just one theme, for example with social icons in widgets on every theme that has widget areas, then the fix is definitely appropriate for core.
But if the CSS that appends a | is coming from a theme or a plugin, then it should not be fixed here, it should be fixed at the source. Makes sense?
There was a problem hiding this comment.
That makes sense! A few themes are guilty of similar styling, but yes, it's from the theme: https://github.com/Automattic/themes/blob/012258cc65cfdfc99bc32117ef951f2c9dbb7189/apostrophe-2/style.css#L1411-L1415
I don't have a strong view either way as to whether there's enough themes affected to justify a Core fix, so I've removed that style from this PR - I'll fix it in the individual themes instead. :)
There was a problem hiding this comment.
Thanks for removing. It's definitely important to not fix at the source, because like I said in some cases we use the pseudo selectors for other things, so this'll have to be a local fix.
There was a problem hiding this comment.
Makes total sense! Please let me know if there are any issues with the remaining changes. :)
There was a problem hiding this comment.
Looks good, hopefully @apeatling can give it a green check!
|
I can't deeply test this morning, but at a glance this looks good. Left one question as a comment. |
apeatling
left a comment
apeatling
left a comment
There was a problem hiding this comment.
Restested this in Twenty Seventeen, Astra, OceanWP, and Twenty Twenty Four. All worked well. I think this LGTM!
|
Thank you! |
3 participants
Fixes #38043
Fixes #53077
Fixes Automattic/themes#5345
Fixes Automattic/themes#6532
Fixes Automattic/themes#6817
Partly fixes Automattic/themes#7062
Fixes Automattic/wp-calypso#79942
etc.
What?
Ensures that theme styles do not distort the Social Links block.
Why?
The Social Links block is a list. As a result, it inherits a lot of styling from different themes, especially in the Widget Area where themes tend to style lists more specifically. The block already does a lot to override this, but it should also deal with
marginandpaddingso that it's not distorted on themes.Automattic/themes#5345 and Automattic/themes#6532 have a long list of themes affected by this, so I'd suggest that a Core fix is more appropriate here.
How?
Overrides any margin and padding set by themes.
Testing Instructions
Screenshots or screencast
Before:
After:
