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

USWDS - Utilities: Generate font-[family]-[size] classes #5388

Merged
merged 1 commit into from Aug 18, 2023

Conversation

danbrady
Copy link
Contributor

@danbrady danbrady commented Jul 18, 2023

Summary

Fixed a bug that caused font-[family]-[size] utility classes to not generate font-family rules.

Breaking change

This is not a breaking change.

Related issue

Closes #5385

Related PR

Changelog PR

Problem statement

  1. Using .font-[family]-[size] utility class should set both the normalized size and family of the font.
  2. Currently, using .font-[family]-[size] utility class only sets the normalized size and not the family of the font.
  3. To correctly style text in the current state, a workaround would be to set both .font-[family] and .font-[family]-[size] instead of the single, latter class.

Solution

As part of the 3.5.0 release, utility settings were updates to include [setting]-complete lists that merged the original, "non-complete" with it. However, the font utility that generates the font-[family]-[size] classes was looking at the original, "non-complete" list and not the updated merged list. This PR updates the $font-settings to $font-settings-complete to correctly generate the classes.

Testing and review

  1. Validate .font-[family]-[size] alone correctly sizes and uses the right font.

Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm run prettier:sass to format any Sass updates.
  • Run npm test and confirm that all tests pass.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.

@amyleadem
Copy link
Contributor

Thanks for submitting this PR. I have added a few core team members as reviewers. We will reach out if we have any questions.

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Great catch! Tested this out with the font-[family]-[size] and it looks like it restored all functionality

  • Confirmed change follows patterns of other utilities
  • Utility sets font family and size appropriately
  • No build errors or visual regression

Additionally, I looked up each of the utilities settings maps in _settings-utilities and I did not find any other utilities that needed to be updated to the -complete map similar to this fix. 👍

Copy link
Contributor

@amyleadem amyleadem 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 submitting this fix! And thanks for the detailed PR description. It is very helpful.

This PR looks good to me. I confirmed that the font-[family]-[size] utilities output font-family again.

I used the following code for testing and results are shown in the screenshots below:

<p class="font-mono-lg">Sample mono text</p>
<p class="font-sans-lg">Sample sans text</p>
<p class="font-serif-lg">Sample serif text</p>
<p class="font-code-lg">Sample code text (mono default)</p>
<p class="font-body-lg">Sample body text (sans default)</p>
<p class="font-heading-lg">Sample heading text (serif default)</p>
<p class="font-alt-lg">Sample alt text (serif default)</p>
<p class="font-ui-lg">Sample alt text (sans default)</p>

Before (all entries show in same default font):
image

After:
image

Additionally, I created a changelog entry for this PR and added it to the PR description.

Copy link
Contributor

@mejiaj mejiaj 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 the fix! I was able to confirm this works on all text utility classes, both defaults and aliases.

Copy link
Member

@thisisdano thisisdano 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 the contribution and fix 🎉

@thisisdano thisisdano merged commit 5e53b48 into uswds:develop Aug 18, 2023
4 checks passed
@thisisdano thisisdano mentioned this pull request Aug 18, 2023
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.

USWDS - Bug: font-family-size does not set font-family
5 participants