Skip to content

USWDS - Card: Enable $theme-card-font-family setting #5974

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

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

cathybaptista
Copy link
Contributor

@cathybaptista cathybaptista commented Jul 8, 2024

Summary

Fixed a bug that caused the component to ignore the $theme-card-font-family setting.

Breaking change

This is potentially a breaking change.

Prior to this PR, the card component disregarded the value set in the $theme-card-font-family setting. Because the component will now respect the value of that setting, your implementation of the card component might display with an unexpected font family. If it does, update the value of $theme-card-font-family in your USWDS settings configuration with your expected font family.

Related issue

Closes #5824

Related pull requests

Changelog uswds/uswds-site#2805

Preview link

Card component preview

This preview won't show any changes until the style itself changes, refer to the screen captures below in the "solution" section to see what happens when this change is included.

Problem statement

$theme-card-font-family is a value that is set in _settings-components.scss. However, the typeset mixin that is used by the style doesn't currently have this value passed to it, so if the style were to change it would not display.

Solution

Added the value of $theme-card-font-family to the @typeset mixin so that whatever value is set in settings is honored in the component. Now, when the $theme-card-font-family is set the component will show the appropriate font-family.

change @include typeset; to @include typeset($theme-card-font-family); so that the value of the style is passed to the mixin.

both of these screen captures use $theme-card-font-family: "mono" !default;

before:
Screenshot 2024-07-08 at 4 08 11 PM
after:
Screenshot 2024-07-08 at 4 08 48 PM

This approach was chosen because if this style changes it should be available in the component.

Testing and review

  • For testing, change $theme-card-font-family to some visually obvious font, such as mono.
  • Open preview link and confirm that the style changes for the card component.

To reproduce:

  1. pull the branch cb-component_should_use_theme_setting_font
  2. this issue appears on card component.
  3. preview will not immediately show this change. To see it, change $theme-card-font-family to some visually obvious font, such as mono and reload the component.
  4. I am just looking for confirmation that it works.
  • 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.
    -->

@cathybaptista cathybaptista marked this pull request as ready for review July 8, 2024 21:08
@cathybaptista cathybaptista changed the title Added theme-card-font-family variable to card so that its value will … USWDS - Card: Enable $theme-card-font-family setting Jul 9, 2024
@amyleadem amyleadem added this to the uswds 3.9.0 milestone Jul 12, 2024
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.

Looks good to me! Thanks for fixing this @cathybaptista.

thought: The only hesitation I have here is whether we should expect the button styles to change as well, since the footer and button are included in usa-card__container and do not have an explicit font family setting like header does. I ultimately decided that it would be too opinionated to force that, and users can easily access the container font for button by adding a font-family: inherit style rule for usa-button if desired.

I was able to:

  • Confirm that there were no visual regressions with the default component
  • Confirm that I could update the value for $theme-card-font-family and see the font update in the body

Note

I added the related issue (#5824) to the 3.9.0 milestone since this seems like a nice quick win to include.

@amyleadem amyleadem requested a review from mejiaj July 12, 2024 15:40
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.

Working like a charm! Great simple fix here. Thank you for including testing instructions as it made this PR easy to review and see the changes take place.

Since we're likely to include this in 3.9.0 it would be great to have a changelog item for this on site! I'll reach out directly about walking you through this process 👍

@mejiaj mejiaj requested review from thisisdano and removed request for mejiaj and RachelCorsino July 15, 2024 15:40
@mejiaj
Copy link
Contributor

mejiaj commented Jul 15, 2024

Changelog entry is pending, but that doesn't have to block the feature PR review.

@cathybaptista please create a changelog entry on the website and add the link to your PR description.

Copy link
Contributor

@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.

Love a nice focussed change like this one

@thisisdano thisisdano merged commit 91c741c into develop Oct 3, 2024
6 checks passed
@thisisdano thisisdano deleted the cb-component_should_use_theme_setting_font branch October 3, 2024 03:57
@thisisdano thisisdano mentioned this pull request Oct 4, 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.

USWDS - Bug: $theme-card-font-family setting does not affect the card component.
6 participants