-
Notifications
You must be signed in to change notification settings - Fork 3
Improved Comment Styles #2022
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
Improved Comment Styles #2022
Conversation
🦋 Changeset detectedLatest commit: 93bf2e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for cloudfour-patterns ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Left one nit comment, LGTM otherwise! 🎉
.wp-block-code, | ||
.wp-block-jetpack-markdown pre, | ||
.legacy-post pre { | ||
.legacy-post :not(.c-comment) > :not(.c-comment__content) > pre { |
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 trying to make sure I understand this:
- Target
pre
elements - That are the direct child of an element that's not
.c-comment__content
- As long as that parent element is not the direct child of
.c-comment
A couple of questions:
- Is that third rule necessary? Could this line be switched to
.legacy-post :not(.c-comment__content) > pre
? - Will our legacy posts always have
pre
elements nested deeply enough for this to work? (a.k.a will there always be two layers of elements in between.legacy-post
and thepre
we want to style?
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.
-
Good point — I can drop the
:not(.c-comment) >
part of the selector. -
Yeah, should be —
legacy-post
is applied to the outermosto-container__content
, andpre
elements will either be part of a blog post or a comment, and in both cases nested deeper in the DOM from that point.
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.
Cool, I just had one other thought about a potential edge case. Would this break if the pre
was nested deeper within a comment. For example, imagine the following comment:
> You wrote this code:
>
> ```js
> console.log('boop');
> ```
>
> I think you meant `beep`
In this scenario the pre
would not be a direct child of .c-comment__content
so wouldn't get the correct styles.
This is a total edge case but it makes me wonder if it would be safer to just un-set the relevant styles when a pre
is in a comment?
.c-comment pre {
margin-inline: 0;
padding-inline: 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.
LGTM!
Overview
This PR updates some of the core block styles to better match base
element styles between posts and comments. Specifically:
Screenshots
Testing
thead
)legacy-post
class to the#root
element.legacy-post
would do this)