Skip to content
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.

Sign up for GitHub

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

Social Links Block: Prevent Theme Styles Distorting Size #56301

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Social Links Block: Prevent Theme Styles Distorting Size #56301

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c047949
Social Links Block: Prevent Theme Styles Distorting Size
Aurorum Nov 19, 2023
6365c30
Remove :after selector
Aurorum Dec 1, 2023
2d870ad
Fix #53077
Aurorum Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions packages/block-library/src/social-links/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,23 @@
}

// This needs specificity because themes usually override it with things like .widget-area a.
.wp-block-social-links .wp-block-social-link .wp-block-social-link-anchor {
&,
&:hover,
&:active,
&:visited,
svg {
color: currentColor;
fill: currentColor;
.wp-block-social-links .wp-block-social-link.wp-social-link {
margin: 0;
padding: 0;

.wp-block-social-link-anchor {
&,
&:hover,
&:active,
&:visited,
svg {
color: currentColor;
fill: currentColor;
}
}

&:after {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is definitely valid, my question is mostly around this pseudo element. Do you know where this code is coming from?

Screenshot 2023-12-01 at 10 51 08

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense! Please let me know if there are any issues with the remaining changes. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, hopefully @apeatling can give it a green check!

display: none;
}
}

Expand Down
Loading