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

Jump to bottom

Fix: Line-height across the app for H1, H2, H3, H4 #1610

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Fix: Line-height across the app for H1, H2, H3, H4 #1610

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2a47b4
feat(line-height): add heading, h4 line-height vars and use for h1-h4
machikoyasuda Aug 2, 2023
c363285
refactor(css): use h4 line-height for card, delete card lh declaration
machikoyasuda Aug 2, 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
18 changes: 12 additions & 6 deletions benefits/static/css/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
--bs-font-sans-serif: Roboto, system-ui, -apple-system, "Segoe UI", sans-serif,
"Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji";
--bs-body-font-family: var(--bs-font-sans-serif);
--bs-body-line-height: 1.5;
--bs-body-line-height: 1.5; /* Displayed as 150% on Figma */
--heading-line-height: 1.3; /* Displayed as 130% on Figma */
--h4-line-height: 1.2; /* Displayed as 120% on Figma */
--body-letter-spacing: 0.05em;
--dark-color: #212121;
--hover-color: #044869;
Expand Down Expand Up @@ -162,7 +164,6 @@ a[target="_blank"]::after {

/* Headlines */
/* All headlines */
/* All headlines share font-weight, letter-spacing, line-height and margin */
h1,
h2,
.h2,
Expand All @@ -171,7 +172,6 @@ h3,
h4 {
font-weight: var(--bold-font-weight);
letter-spacing: var(--body-letter-spacing);
line-height: var(--bs-body-line-height);
margin: 0;
}

Expand All @@ -183,6 +183,7 @@ h1 {
font-size: var(--h1-font-size);
text-align: var(--h1-text-align);
padding-top: calc(70rem / 16);
line-height: var(--heading-line-height);
}

/* H2 */
Expand All @@ -191,6 +192,7 @@ h1 {
h2,
.h2 {
font-size: var(--h2-font-size);
line-height: var(--heading-line-height);
}

/* H3 */
Expand All @@ -199,6 +201,13 @@ h2,
h3,
.h3 {
font-size: var(--h3-font-size);
line-height: var(--heading-line-height);
}

/* H4 */
/* Desktop only: Used for Agency Selector card, agency name */
h4 {
line-height: var(--h4-line-height);
}
Comment on lines +207 to 211
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


/* Main */
Expand Down Expand Up @@ -606,7 +615,6 @@ h1 + .media-list, /* A .media-list immediately following the h1: Enrollment Succ

:root {
--card-title-font-size: var(--font-size-18px);
--card-title-line-height: calc(21.6rem / 16);
--card-title-letter-spacing: 0.03em;
--card-x-padding: calc(10rem / 16);
--card-body-x-padding: 0;
Expand All @@ -619,7 +627,6 @@ h1 + .media-list, /* A .media-list immediately following the h1: Enrollment Succ
@media (min-width: 992px) {
:root {
--card-title-font-size: var(--font-size-20px);
--card-title-line-height: calc(24rem / 16);
--card-title-letter-spacing: 0.05rem;
--card-x-padding: calc(35rem / 16);
--card-body-x-padding: calc(40rem / 16);
Expand Down Expand Up @@ -654,7 +661,6 @@ a.card:focus-visible {

.card .card-title {
font-size: var(--card-title-font-size);
line-height: var(--card-title-line-height);
letter-spacing: var(--card-title-letter-spacing);
}

Expand Down
Loading