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

Cap heights for the theme typeface in USWDS #1157

Closed
wants to merge 3 commits into from

Conversation

dzole0311
Copy link
Contributor

@dzole0311 dzole0311 commented Sep 17, 2024

Related Ticket: #1144

Description of Changes

USWDS normalizes font sizes across different typefaces to ensure optical consistency, but this feature isn't necessary for our current needs. Here we're trying to streamline the font sizes across all typefaces to ensure they have uniform sizing without normalization.

I found the function where USWDS normalizes the typefaces, which is based on the cap-height of each typeface. Since each typeface has a slightly different cap-height setting, I bypassed this by configuring the custom tokens that we need to use a fixed default cap-height of 364px.

To verify the changes, I tested with the following examples:

<h1 className='font-serif-3xs'>Lorem ipsum dolor sit amet, consectetur adipiscing elit </h1>
<h2 className='font-sans-3xs'>Lorem ipsum dolor sit amet, consectetur adipiscing elit </h2>

These previously would have been normalized differently, but now output the same rem value.

Notes & Questions About Changes

{Add additonal notes and outstanding questions here related to changes in this pull request}

Validation / Testing

The example from the description can be used for testing

@dzole0311 dzole0311 marked this pull request as draft September 17, 2024 11:51
Copy link

netlify bot commented Sep 17, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit f3146cf
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66e96d38f8c50c00086300de
😎 Deploy Preview https://deploy-preview-1157--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor Author

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

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

There should be no risk if we decide to merge these updated configs, as I think no visual changes will appear.

),
"merriweather": (
display-name: "Merriweather Web",
cap-height: 364px,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a way to only set the cap-height without adding the rest of the values for the typeface, copied from the uswds config defaults.

900: false,
),
),
),
),
$theme-type-scale-md: 8,
$theme-utility-breakpoints: (
"mobile-lg": false,
"desktop": false
),
$theme-font-type-sans: baseFontFamily,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@faustoperez I mapped sans to the base font family (e.g "Inter" in GHG Center's case). Could be adjusted as we need it.

@dzole0311 dzole0311 marked this pull request as ready for review September 18, 2024 09:57
@hanbyul-here
Copy link
Collaborator

Thanks a lot for working on it 🙇 It is quite amount of setups just to disable the normalization of the font - I end up having this question if we need to get rid of this normalization at all (I know, I am sorry 😓 ) I think we started this work because we don't need this normalization - but it might be ok to have the normalization of the font? @faustoperez - What do you think?

@faustoperez
Copy link

@hanbyul-here sounds good! I think we should pause the USWDS detailed decisions work (ie should we normalize the font sizes) until we have more clarity on the vision/template instance design (quite likely at the end of this week)

@dzole0311
Copy link
Contributor Author

@faustoperez @hanbyul-here thanks for the feedback. I'll close this PR and the related issue now, since now we have a working implementation and reference if we decide to remove the normalization again.

@dzole0311 dzole0311 closed this Sep 23, 2024
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