Visitar URL original
USWDS - [usa-icon-list] Add support for $theme-body-font-size to be 2xs by paulathevalley · Pull Request #5356 · uswds/uswds · GitHub
Skip to content

USWDS - [usa-icon-list] Add support for $theme-body-font-size to be 2xs - #5356

Closed
paulathevalley wants to merge 1 commit into
uswds:developfrom
paulathevalley:develop
Closed

USWDS - [usa-icon-list] Add support for $theme-body-font-size to be 2xs#5356
paulathevalley wants to merge 1 commit into
uswds:developfrom
paulathevalley:develop

Conversation

@paulathevalley

@paulathevalley paulathevalley commented Jun 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Allow $theme-body-font-size to be set to 2xs in uswds-theme.

Breaking change

This is not a breaking change.

Related issue

Closes #5357

Problem statement

Summarize the problem this PR solves in a clear and concise statement.

Setting $theme-body-font-size to '2xs' unexpectedly throws an error, blocking the page render.

Solution

Provide a summary of the solution this PR offers.

This PR:

  • Add '2xs': $theme-type-scale-2xs to the list of available font-sizes

There may be a more robust way to grab all the possible type-scale theme variables instead of manually updating them by key here. I am also unclear whether there was a conscious decision to support a limited set of sizes.

In our use case, the $theme-type-scale-2xs is set to token 6 which is roughly 17px.

Testing and review

Share recommended methods for reviewing this change.

In _uswds-theme.scss:

@use 'uswds-core' with (
  $theme-body-font-size: '2xs'
)

Error message is resolved and page loads.

@paulathevalley
paulathevalley marked this pull request as ready for review June 29, 2023 18:24
@mejiaj
mejiaj requested review from amyleadem, mahoneycm and mejiaj June 30, 2023 14:51
mahoneycm
mahoneycm approved these changes Jun 30, 2023
View reviewed changes

@mahoneycm mahoneycm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for flagging and contributing! This is working as expected for me!

  • Setting $theme-body-font-size: "2xs" doesn't result in any build errors
  • Components that use $theme-body-font-size are updated correctly without any negative visual regression
  • Sass functions that use $theme-body-font-size accept the value and do not error

@mejiaj @amycole501 Looking ahead, would it be in our best interest to include "3xs": $theme-type-scale-3xs to cover all of the Type scale values?

@amyleadem amyleadem self-assigned this Jul 6, 2023
@mahoneycm mahoneycm mentioned this pull request Jul 6, 2023
Merged
This was referenced Jul 7, 2023
Merged
Closed
@amyleadem

amyleadem commented Jul 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@paulathevalley Thank you for submitting this issue and PR. This PR works well, but it more importantly exposed a bigger issue with the component code not fitting current system patterns. To fix this larger issue, I have opened PR #5363 as an alternate approach.

Thanks again for your contribution! I am going to close this PR in favor of #5363, but please let us know if you have questions.

@amyleadem amyleadem closed this Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mejiaj mejiaj Awaiting requested review from mejiaj
@amyleadem amyleadem Awaiting requested review from amyleadem
1 more reviewer
@mahoneycm mahoneycm mahoneycm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@amyleadem amyleadem

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USWDS - Bug: usa-icon-list package does not allow '2xs' for $theme-body-font-size variable

3 participants

@paulathevalley @amyleadem @mahoneycm