-
Notifications
You must be signed in to change notification settings - Fork 353
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
Try: Make spacing overrides more specific in Seedlet #3017
Conversation
This PR also solves #3015, I just tested it. 🙂 |
@@ -6,6 +6,8 @@ | |||
margin-top: inherit; | |||
margin-bottom: inherit; | |||
|
|||
@include innerblock-margin-clear(".wp-block-column"); |
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.
I'm not sure how well I'm following all this either, but I don't think so, and it doesn't seem to. If @kjellr wouldn't mind taking a glance at this, I'd much appreciate it.
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.
Update: I've noticed that in viewing the post itself, there wouldn't be a top margin on those paragraphs, although there seem to be quite a few discrepancies when it comes to vertical spacing on the editor vs the post itself (both with, and without this PR), so I'm not sure how far we want to go in trying to rectify that as part of this PR.
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.
Gotcha. In that case, is this mixin needed at all?
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.
Haha. 🤔 Maybe not? I copied this implementation from Twenty Twenty One because I had assumed this was, for some reason, desirable.
I have to confess I couldn't actually work out exactly what this achieves on Twenty Twenty One either, but presumed I was missing something and we needed to keep it.
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.
If it's not needed let's leave it out. Keep it simple!
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'll make this change.
@@ -149,15 +149,6 @@ | |||
[data-block] { | |||
margin-top: var(--global--spacing-vertical); | |||
margin-bottom: var(--global--spacing-vertical); | |||
|
|||
[data-block]:first-child { |
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 can confirm that removing these lines solves the social icons + buttons alignment issue in the editor.
PR has been amended to reflect that we're just going with dropping the global margin settings as we don't think they're needed. So I think this is now ready to land subject to a final approval. |
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.
LGTM
This PR solves #2727 by rewriting some CSS rules that are unnecessarily global, as was done in Twenty Twenty One.
If this is a goer, we'll want to propagate this change to Seedlet's child themes.
Unlike Twenty Twenty One, I think there's just one instance where the specificity needs to be added because Seedlet has fewer editor tweaks than Twenty Twenty One. I used the same mixin approach as before anyway in case I'm wrong, and to ease adding this specific override anywhere else that needs it.