Tidy up Post Comments default styling #37709
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kjellr
added
[Type] Bug
kjellr
commented
Jan 4, 2022
|
|
||
| b { | ||
| font-weight: normal; | ||
| } |
There was a problem hiding this comment.
I know this feels really un-semantic, but we can't change the markup. 😕 Happy to remove this part if folks feel strongly. But I think it really sticks out if a theme doesn't use bold text elsewhere in the UI.
There was a problem hiding this comment.
When you say "we can't change the markup", take it you mean in a back-compat sense? To me the part of this that feels unsemantic is the use of the b tag, which honestly does feel worth changing (removing?) right at the source through a core patch, that's the only semi-strong feeling I have. Alternatively, would a font-weight: 500; change be more palpable? It's 100 more than the default, but technically still "bolder".
There was a problem hiding this comment.
Yeah, I guess we can change the markup. I just hadn't really thought about it. 🤔
In any case, I could change this to inherit instead of normal and it should usually work fine. Do you think that would be any better than specifying normal here?
There was a problem hiding this comment.
As noted, it's not a super strong feeling, mostly because the semantic concern comes from the fact that the tag reads as "bold", which is kind of beyond interpretation, whereas something like strong would leave wiggleroom for how that gets visually interpreted. But we're way in the weeds on nitpickiness and not something I think should block the PR. If anything, we could add a comment that the b tag should be fixed at the source, but even that I'm not sure about.
There was a problem hiding this comment.
Ok, makes sense. I think we can just move forward with this as is, and change it later on if more folks have concerns about it.
|
Size Change: +70 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
jasmussen
approved these changes
Jan 10, 2022
| line-height: 1.8; | ||
| margin: 0.36em 0 1.4em; | ||
| margin: 1em 0; |
There was a problem hiding this comment.
Very nice, 1em is profoundly easier to work with, yet still provides preditable inheritance based styling opportunities.
| } | ||
|
|
||
| .comment-body .commentmetadata { | ||
| font-size: 0.75em; | ||
| font-size: 0.875em; |
There was a problem hiding this comment.
This change unifies with the comment-meta on the top level comment, and the reply button, nice. That said, 0.75 both feels like simpler em math, but also felt visually better to me: 0.875 is smaller, but it doesn't look smaller enough to be intentional. This is not a strong feeling.
There was a problem hiding this comment.
Yeah, these numbers all seem a little random. 😅 The reason I went with the higher value of 0.875em here is because it's slightly larger, and I thought that would be a safer route.
For example, if 1em here were the typical browser default value of 16px, then 0.875em turns out to be 14px, and 0.75em is 12px. 12px seems way too small for anything these days. 14px is still small but better at least.
|
|
||
| b { | ||
| font-weight: normal; | ||
| } |
There was a problem hiding this comment.
When you say "we can't change the markup", take it you mean in a back-compat sense? To me the part of this that feels unsemantic is the use of the b tag, which honestly does feel worth changing (removing?) right at the source through a core patch, that's the only semi-strong feeling I have. Alternatively, would a font-weight: 500; change be more palpable? It's 100 more than the default, but technically still "bolder".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the change that resolved #37464, we lost some of the comments form refinement that was introduced in #30382. It looks a bit messy at the moment. This PR takes a quick stab at tidying things up again.
In the long term, this should go away in favor of the comments query loop. But fixing this would be helpful in the meantime.