Skip to content

USWDS - Icon list: Allow $theme-body-font-size to accept all size tokens #5363

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.

Sign up for GitHub

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

Jump to bottom
Merged
thisisdano merged 3 commits into from
Aug 11, 2023
Merged

USWDS - Icon list: Allow $theme-body-font-size to accept all size tokens #5363

thisisdano merged 3 commits into from
Aug 11, 2023

Conversation

@amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Jul 7, 2023 •
edited
Loading

Summary

Updated icon list styles to allow $theme-body-font-size to accept 2xs and 3xs size tokens.

Breaking change

This is not a breaking change.

Related issue

Issue #5357

Related PR

Changelog and documentation update PR

Preview link

Preview link: Icon list component

Problem statement

Icon list styles are preventing users from setting $theme-body-font-size to 2xs or 3xs size tokens.

Solution

Currently, the icon list styles rely on a component-specific map ($theme-body-font-sizes) to translate acceptable token values. This map does not include 2xs or 3xs sizes.

Because icon list styles rely on $theme-body-font-size as a fallback when it can't find a value in $theme-body-font-sizes, it doesn't know what to do when $theme-body-font-size is set to 2xs or 3xs. It cannot translate the value.

To allow these missing size tokens, as well as remove the need for maintaining a component-specific map, this PR uses the system $all-type-scale in place of $theme-body-font-sizes. The $all-type-scale variable in uswds-core combines $project-type-scale and $system-type-scale maps.

Considerations

⚠️ We should consider that it might have been an intentional choice to exclude 2xs and 3xs from the icon list sizes. If so, we should build an alternate solution.

Note
After team discussion, we determined that there is no need to take an opinionated stance here. Instead, we should consider how to provide guidance on readable text and icon size.

Testing and review

  • Open this branch and add custom values to $theme-body-font-size, including "2xs" and "3xs"

    Note
    These are the stated accepted values: micro, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 3xs, 2xs, xs, sm, md, lg, xl, 2xl, 3xl

  • Confirm there are no compilation errors with these values
  • Check custom sizes for visual regressions against develop

@amyleadem amyleadem requested review from mahoneycm and mejiaj July 7, 2023 18:53
@amyleadem amyleadem mentioned this pull request Jul 12, 2023
Closed
2 tasks
@amyleadem amyleadem changed the title [Demo] USWDS - Icon list: Fix icon size error USWDS - Icon list: Fix icon size error Jul 13, 2023
This was referenced Jul 13, 2023
Closed
Merged
@amyleadem amyleadem linked an issue Jul 14, 2023 that may be closed by this pull request
USWDS - Bug: usa-icon-list package does not allow '2xs' for $theme-body-font-size variable #5357
Closed
2 tasks
@amyleadem amyleadem marked this pull request as ready for review July 14, 2023 15:56
This was referenced Jul 14, 2023
Merged
Closed
@amyleadem amyleadem changed the title USWDS - Icon list: Fix icon size error USWDS - Icon list: Fix bug that prevented $theme-body-font-size from accepting 2xs, 3xs Jul 18, 2023
@amyleadem amyleadem changed the title USWDS - Icon list: Fix bug that prevented $theme-body-font-size from accepting 2xs, 3xs USWDS - Icon list: Allow $theme-body-font-size to accept all size tokens Jul 18, 2023
mahoneycm
mahoneycm approved these changes Jul 18, 2023
View reviewed changes
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.

Intuitive and effective!

  • Updating $theme-body-font-size with 2xs and 3xs correctly updated icon list
  • No compilation errors with listed values
  • Compilation errors correctly when using an unaccepted value
  • No noted visual regression

mejiaj
mejiaj approved these changes Aug 7, 2023
View reviewed changes
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.

Nice fix, all type scale settings are now supported!

Warning
Added note about increased size in compiled CSS file at the end.

Items tested

  • No compile errors
  • Icon list generates sizes based on all size tokens 1

Alternate testing

You can visit the variant Icon List Custom Size and change icon size via StorybookJS controls.

Storybook adds a textarea you can use to test the generated values.

Example screenshot

image
Values tested

uswds/packages/uswds-core/src/styles/tokens/font/type-scale.scss

Lines 1 to 23 in 0395273

$system-type-scale: (
"micro": 10px,
1: 12px,
2: 13px,
3: 14px,
4: 15px,
5: 16px,
6: 17px,
7: 18px,
8: 20px,
9: 22px,
10: 24px,
11: 28px,
12: 32px,
13: 36px,
14: 40px,
15: 48px,
16: 56px,
17: 64px,
18: 80px,
19: 120px,
20: 140px,
);

uswds/packages/uswds-core/src/styles/variables/type-scale.scss

Lines 1 to 18 in d5a7fb0

@use "../functions/general";
@use "../functions/font/system-type-scale" as *;
@use "../settings";
@use "../tokens/font/type-scale" as *;
$project-type-scale: (
"3xs": system-type-scale(settings.$theme-type-scale-3xs),
"2xs": system-type-scale(settings.$theme-type-scale-2xs),
"xs": system-type-scale(settings.$theme-type-scale-xs),
"sm": system-type-scale(settings.$theme-type-scale-sm),
"md": system-type-scale(settings.$theme-type-scale-md),
"lg": system-type-scale(settings.$theme-type-scale-lg),
"xl": system-type-scale(settings.$theme-type-scale-xl),
"2xl": system-type-scale(settings.$theme-type-scale-2xl),
"3xl": system-type-scale(settings.$theme-type-scale-3xl),
);
$all-type-scale: general.map-collect($system-type-scale, $project-type-scale);

Footnotes

  1. ⚠️ Warning: Compiled CSS file size increases.620KB on develop vs 659KB on al-icon-list-scale ↩

@mejiaj mejiaj requested a review from thisisdano August 7, 2023 20:15
@amyleadem amyleadem mentioned this pull request Aug 8, 2023
Closed
thisisdano
thisisdano approved these changes Aug 11, 2023
View reviewed changes
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.

Good callout on the compiled CSS size increase, though I think this fix is good to go as-is and the impact on minified CSS should be minimal

@thisisdano thisisdano merged commit 151873d into develop Aug 11, 2023
@thisisdano thisisdano deleted the al-icon-list-scale branch August 11, 2023 16:05
@thisisdano thisisdano mentioned this pull request Aug 18, 2023
Merged
@sennhann29 sennhann29 mentioned this pull request Jul 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@mejiaj mejiaj mejiaj approved these changes

@thisisdano thisisdano thisisdano approved these changes

@mahoneycm mahoneycm mahoneycm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

5 participants

@amyleadem @mejiaj @thisisdano @mahoneycm