Fix: Line-height across the app for H1, H2, H3, H4 #1610
Conversation
github-actions
bot
added
front-end
|
@srhhnry How does this change look to you? Here's my comment summarizing my findings for both line-height and letter-spacing: #1600 (comment) This PR deals exclusively with line-height. I decided to get that one out first since it's simpler than letter-spacing. |
| /* H4 */ | ||
| /* Desktop only: Used for Agency Selector card, agency name */ | ||
| h4 { | ||
| line-height: var(--h4-line-height); | ||
| } |
There was a problem hiding this comment.
One could argue we should be declaring the H4 font sizes here, but I opted not to for this PR. I did add that comment, thought, that it's only used on Agency Selector card title. Next time we do use the H4, it can be refactored.
There was a problem hiding this comment.
I get that you're defining line-height on h4 because the Agency Selector card title is the only place we actually use an h4 element. And here you're saying that, by the same reasoning, we could also define all the styles for the Agency Selector card title on h4.
I want to note that this is slightly different than what Figma says, since the Style Guide has h4 with a certain font-size, line-height, etc. and the Components page has the Agency Selector Cards with a certain font-size, line-height, etc.
This is fine though since this code change gives us the net-effect we want and, as you noted,
Next time we do use the H4, it can be refactored.
There was a problem hiding this comment.
Yea the next Letter-Spacing PR will apply the h4 letter-spacing styles to this h4, and then the Agency Selector's H4 will get those styles.
And we know we're gonna refactor the entire modal layout/includes after #1444 this ticket for SBMTD, we can revisit how the h4 on the modal is constructed and styled more thoroughly then.
| /* H4 */ | ||
| /* Desktop only: Used for Agency Selector card, agency name */ | ||
| h4 { | ||
| line-height: var(--h4-line-height); | ||
| } |
There was a problem hiding this comment.
I get that you're defining line-height on h4 because the Agency Selector card title is the only place we actually use an h4 element. And here you're saying that, by the same reasoning, we could also define all the styles for the Agency Selector card title on h4.
I want to note that this is slightly different than what Figma says, since the Style Guide has h4 with a certain font-size, line-height, etc. and the Components page has the Agency Selector Cards with a certain font-size, line-height, etc.
This is fine though since this code change gives us the net-effect we want and, as you noted,
Next time we do use the H4, it can be refactored.
|
Late on this comment but line height looks fine to me from that screenshot. |
|
Awesome thank you @srhhnry. This PR is merged into |
fixes #1573
Line-height should always be a unitless value. https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#values Do not use rems, pixels for line-height.
What this PR does
Steps I took to run through this PR
--bs-body-line-heightvariable is correct, set at 1.5, and applied to all the places it should.--heading-line-heightof value 1.3.--h4-line-heightof value 1.2. H4 is only used in one spot in the whole app, and that is for the Agency Selector modal, specifically, the name of the transit agency. The 1.2 value allows for the transit agency name to be tighter.--heading-line-heightto H1, H2, H3--h4-line-heightoutlier exception to H4..card .card-title's line-height, which is an H4, to fall back and use the H4 line-height.Is this even noticeable? Yes.
Modal titles on Mobile
Side-by-side comparison of Figma, this PR and the Dev site for modal titles. Modal titles on Dev have a line-height of 1.5, and now, with this PR, they are tightened to 1.3.
Index/Agency Index on Desktop
Total height of the H1 and H2 types are tighter now
2-line Headlines on Desktop