Skip to content

Remove instances of "h1" BassCSS class#5614

Merged
aduth merged 2 commits intomainfrom
aduth-rm-h1
Nov 18, 2021
Merged

Remove instances of "h1" BassCSS class#5614
aduth merged 2 commits intomainfrom
aduth-rm-h1

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 17, 2021

Why: Toward migration off BassCSS to design system.

The font-sans-lg design system utility class provides a similar font size styling effect (1.5rem with h1 vs. 1.46rem with font-sans-lg). In the future, we may consider avoiding these classes altogether, toward more semantic elements such as headings, or componentizing specific usage such as process lists (LG-4791).

Open Questions:

  • The font size for the "Upload" step in the Figma design is 1.25rem, which matches neither h1 nor font-sans-lg. Does that font size correspond to anything meaningful (component, etc)? Should it be customized to match the design, or should the design be updated to use the design system font size token?

(cc @anniehirshman-gsa)

Screenshots:

Screen Before After
PIV Setup piv-before piv-after
IAL2 Device Choice upload-before upload-after

**Why**: Toward migration off BassCSS to design system.
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Thanks for surfacing this! I agree that it'd be good to make these semantic headings even if the large text size changes a little. In fact, I updated the Upload page heading to an h2 in mockups for a design improvement that hasn't been implemented yet (Figma, LG-5196).

The only trick is that with the current font set, h2s are serifs and it would be preferable to keep them sans. Do you have a good suggestion for a temporary fix for that?

@aduth
Copy link
Contributor Author

aduth commented Nov 17, 2021

The only trick is that with the current font set, h2s are serifs and it would be preferable to keep them sans. Do you have a good suggestion for a temporary fix for that?

Yeah, we can use the font-family-sans utility class to force it to render in sans-serif font, i.e. <h2 class="font-family-sans" />. It'd become redundant once we switch to Public Sans, at which point we'd just want to return and remove it.

For reference, this is what it looks like with that change:

Before (current branch changes) After
Screen Shot 2021-11-17 at 3 56 34 PM after screenshot

I can go ahead and make the change if it looks alright to you? I think it does match what's in the current Figma design, rendering at 1.25rem.

@anniehirshman-gsa
Copy link
Contributor

I can go ahead and make the change if it looks alright to you? I think it does match what's in the current Figma design, rendering at 1.25rem.

LGTM thanks! 👍

@aduth aduth merged commit a6bd778 into main Nov 18, 2021
@aduth aduth deleted the aduth-rm-h1 branch November 18, 2021 15:03
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.

3 participants