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.

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

acf heading style updates #139

Merged
merged 3 commits into from
Jan 3, 2025
Merged

acf heading style updates #139

merged 3 commits into from
Jan 3, 2025

Conversation

jtmst
Copy link
Collaborator

@jtmst jtmst commented Dec 31, 2024

issue: #130

Modifies headers to match new spec. h1, h4, and h7 are modified for the ACF theme

BEFORE:
Screen Shot 2024-12-31 at 3 21 44 PM
Screen Shot 2024-12-31 at 4 01 41 PM

AFTER:
Screen Shot 2024-12-31 at 3 58 41 PM

Screen Shot 2024-12-31 at 3 45 00 PM Screen Shot 2024-12-31 at 4 01 41 PM

@jtmst jtmst requested a review from pcraig3 December 31, 2024 21:09
Copy link
Collaborator

@pcraig3 pcraig3 left a comment

Choose a reason for hiding this comment

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

Very minor changes requested, but otherwise looks good.

I had thought we only got 2 font weights bundled with the USWDS stuff, so glad to see that adding a new one is simple.

Comment on lines +148 to +156
/* Add source sans pro */
@font-face {
font-family: "Source Sans Pro Web";
font-style: normal;
font-weight: 600;
font-display: fallback;
src: url(/static/fonts/source-sans-pro/sourcesanspro-semibold-webfont.woff2) format("woff2");
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am surprised this is there! Good find 👍

Comment on lines +41 to +42
font-size: 36pt;
font-weight: 700;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -51,9 +53,11 @@ h3 {
}

h4 {
color: var(--color--cdc-blue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's delete this — it gets overwritten in theme-opdiv-acf-white.css anyway.

font-family: Source Sans Pro Web, Helvetica Neue, Helvetica, Roboto, Arial,
sans-serif;
color: var(--color--cdc-blue);
font-size: 20pt;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is already set to 20pt in theme-base.css

font-size: 11.5pt !important;
font-weight: 600 !important;
color: var(--color--acf-blue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these need !important?

jtmst and others added 3 commits January 3, 2025 10:09
- h4 size is already set in theme-base.css
- h4 colour in theme-opdiv-acf.css is overwritten later
- h7 styles don't need to be !important
@pcraig3 pcraig3 force-pushed the update-acf-heading-styles branch from c83ae30 to 2382562 Compare January 3, 2025 15:11
Copy link
Collaborator

@pcraig3 pcraig3 left a comment

Choose a reason for hiding this comment

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

Amazing work! Merging now.

@pcraig3 pcraig3 merged commit 84eda82 into main Jan 3, 2025
4 checks passed
@pcraig3 pcraig3 mentioned this pull request Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants