USWDS - Card: Fix `$theme-card-border-width` calculation errors #5325

mahoneycm · 2023-06-07T18:20:53Z

Summary

Fixed a bug that prevented $theme-card-border-width from accepting 0 or string tokens.

Breaking change

This is not a breaking change.

Related issue

Closes #5162

Related pull requests

Changelog entry →

Preview link

Card storybook preview →
Test repo →

Problem statement

Setting `$theme-card-border-width` to zero

This caused errors when setting the card radius using the calc() function because calc() cannot take unitless numbers¹

Setting `$theme-card-border-width` to a string token

Exdent variants of card use the variable to set a negative border width the following calculation:

@include u-margin-x(-$theme-card-border-width);

When using a string token like "05", this results in u-margin-x(-"05") which breaks the margin function because it is not a valid USWDS token

Solution

Update $system-spacing.special 0 value from 0: 0 to 0: spacing-multiple(0)
- This falls in line with other $system-spacing tokens
  - "05": spacing-multiple(0.5)
  - 1: spacing-multiple(1),
- units() is the only function that references $project-spacing-standard
- Resolves this issue anywhere units(0) is used within a calc() function
  - Across USWDS, sass interpolation (#{}) is used within calc() to maintain the rem unit passed in by the units() function
  - #{units($theme-card-border-width)} results in 0rem
  - Other references to units(0) still result in unitless 0
Update method to get negative margins to work with string token values
- Changed to a solution found in _footer.scss
- Works with all width tokens mentioned in _border
Swap u-margin mixins for native solutions where $theme-card-border-width is passed in.
- The margin mixins expect USWDS tokens and will flag an error when it doesn't receive one

- @include u-margin-x(-$theme-card-border-width);
+ magin-inline: units($theme-site-margins-mobile-width * -1);

Testing and review

Confirm bugs exist
- On develop, set $theme-card-border-width to 0
- Inspect .usa-card__img and verify calc() is not working correctly
- Set $theme-card-border-width to "05" (or another string token)
- Inspect build error
Checkout USWDS-Sandbox test repo →
- Recent changes have been pushed to this branch, make sure you pull the latest
$theme-card-border-width will be set to 0 by default
Inspect .usa-card__img and verify calc() is working correctly
Go to _uswds-theme.scss and set $theme-card-border-width to "05"
Verify that there are no build errors
Inspect .usa-card__media--exdent class and check negative margins are set correctly

Testing Checklist

$theme-card-border-width: 0 does not break calc() function on .usa-card__img
$theme-card-border-width: "05" does not cause build errors
No unwanted visual regression

Additional info

I discovered an existing visual bug when setting the border to a large value. I created a ticket at #5401

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.

https://sass-lang.com/documentation/values/calculations#:~:text=Other%20calculations%20don%E2%80%99t%20allow%20unitless%20numbers%20to%20be%20added%20to%2C%20subtracted%20from%2C%20or%20compared%20to%20numbers%20with%20units. ↩

amyleadem · 2023-06-27T23:21:05Z

Hi @mahoneycm,
I hit a series of warnings when I set $theme-card-border-width: 0 (They should be visible in your sandbox demo if you set $theme-show-compile-warnings to true).

Warning: `0rem` is not a USWDS `margin-horizontal` token. This is OK, but only components built with standard tokens can be accepted back into the system. Standard `margin-horizontal` values: 1px, 2px, 05, 1, 105, 2, 205, 3, neg-1px, neg-2px, neg-05, neg-1, neg-105, neg-2, neg-205, neg-3, neg-4, neg-5, neg-6, neg-7, neg-8, neg-9, neg-10, neg-15, 4, 5, 6, 7, 8, 9, 10, 15, card, card-lg, mobile, 05em, 1em, 105em, 2em, 0, auto

This happens with many of the accepted token values for the setting. Is it possible to resolve these?

mahoneycm · 2023-07-05T18:29:25Z

@amyleadem
CC: @mejiaj

For context, the rem unit is caused by the spacing-multiple() function I added to the 0 value in $system-spacing.special. The functions that do not require the unit, drop the unit on output to just become 0 (example of this below)

how do we feel about resolving the warning by adding 0rem to the $partial-values map? The warning disappears when this one line is added.

$partial-values: (
  zero-zero: (
    0: 0,
+   0rem: 0,
  ),

or by adding a rem specific map similar to $system-spacing-em:

$system-spacing-em: (
  small: (
    "05em": 0.5em,
    1em: 1em,
    105em: 1.5em,
    2em: 2em,
  ),
);

I see a benefit in this because calc() functions require the unit to operate, so we need to be able to pass 0rem to these functions if we want to allow zero values. Functions that do not require the unit will still return a unitless value, such as

$theme-card-border-width: 0;

.usa-card__media--exdent {
  @include u-margin-right(units($theme-card-border-width) * -1);
  @include u-margin-left(0);
}

Returns:

mejiaj

I see the warnings Amy mentioned above. We should look at potential alternatives.

For context, the rem unit is caused by the spacing-multiple() function I added to the 0 value in $system-spacing.special. The functions that do not require the unit, drop the unit on output to just become 0 (example of this below)

Are you saying this is what's causing the rem warning on compile? The warnings point to examples like this:

  @include u-margin-left(units($theme-card-border-width) * -1);

Which makes sense because u-margin-left expects tokens, not the value of units().

how do we feel about resolving the warning by adding 0rem to the $partial-values map? The warning disappears when this one line is added.

Adding unit specific values, in this case rem seems like a can of worms. Because then we'd have to add in values for em, px, ch, and any other unit that could potentially fail.

Have we investigated the following?

Swapping utility mixins for native properties so we can pass units().
Using internal logic to check for border width values of 0 or none and then passing that to calc().
Alternatively, we could get rid of calc(), but this doesn't seem like a long term solution.

mahoneycm · 2023-07-20T20:16:19Z

@mejiaj swapping the utility mixins for native properties works. Should I follow that pattern throughout the card scss? Or only where calc() is used?

There also seems to be some general cleanup that can be done here. It references usa-card__header--exdent which isn't mentioned on the guidance page and doesn't seem to cause any changes to the placement of the header.

Let me know if you want me to go ahead and swap the native properties and I can push up a PR. I'll remove the exdent class definition as well and we can discuss from there

mejiaj · 2023-07-20T20:47:15Z

I was originally thinking only where calc() is used. Do you see any issues with this approach?

mahoneycm · 2023-07-20T20:48:44Z

@mejiaj Nope! That sounds good to me and removed the errors in my initial testing! I was just curious on where you stood from a code consistency standpoint. I'll work on getting these updated.

Necessary for string tokens to become negative

mahoneycm · 2023-07-25T15:06:24Z

@mejiaj @amyleadem Updated and ready for review! I also updated the PR description and test repo

I also found a visual bug that exists in develop. Reported at #5401

amyleadem

I appreciate the test you built out, @mahoneycm . Always makes it easy.

Looks good to me!

Confirm no compilation or calc errors with string tokens
Confirm no compilation or calc errors with 0 value
Confirm no visual regression
Confirm no compile warnings

mejiaj

Looks good. I didn't find any visual regressions in USWDS or Sandbox branches.

I've tested and can confirm:

Zero visual regressions.
Zero compile errors.
Border width tokens work in $theme-card-border-width¹.

Sandbox testing

Thanks for including a test sandbox branch. I made additional changes locally. Changes include:

Install and compare USWDS latest.
Updating flag default to use media exdent.

⚠️ Documentation for $theme-card-border-width shows units as token, but border width tokens only support $system-spacing maps for `smaller, small, and zero-zero. See _properties.scss, which depends on spacing.scss ↩

mejiaj · 2023-08-02T16:20:44Z

packages/usa-card/src/styles/_usa-card.scss

-        .usa-card__img {
-          @include u-radius(0);
-          @include u-radius-right($theme-card-border-radius);
-        }


Note
The line change on 271 makes this safe to remove.

Tested with both flag exdent media variants in Sandbox. Example with $theme-card-border-width: "105":

mejiaj · 2023-08-02T16:25:38Z

packages/uswds-core/src/styles/tokens/units/spacing.scss

@@ -99,7 +99,7 @@ $system-spacing: (
    // 1400px
  ),
  "special": (
-    0: 0,
+    0: spacing-multiple(0),


Curious as to why this was changed? The commit message doesn't seem to mention why. I just see:

Update 0 spacing unit in system-spacing special map

This one is outlined under the solution section of the PR description. Essentially, it passes the value as 0rem instead of a unitless 0, which is needed for calc(). SCSS does it's magic and drops the rem unit when it is not needed automatically so it'll remain unitless elsewhere.

Update $system-spacing.special 0 value from 0: 0 to 0: spacing-multiple(0)

This falls in line with other $system-spacing tokens

"05": spacing-multiple(0.5)

1: spacing-multiple(1),

units() is the only function that references $project-spacing-standard

Resolves this issue anywhere units(0) is used within a calc() function

Across USWDS, sass interpolation (#{}) is used within calc() to maintain the rem unit passed in by the units() function

#{units($theme-card-border-width)} results in 0rem

Other references to units(0) still result in unitless 0

@mahoneycm thanks for the reply and apologies for missing it. I must've confused it since the formatting is similar to the testing & review list 🤦.

thisisdano

Excellent discussion/review to get to a really good, consistent, and understandable solution

mahoneycm added 3 commits June 7, 2023 11:00

Update 0 spacing unit in system-spacing special map

668cbc7

Update card negative margin calculation to resolve unit error

3e9f817

npm run prettier

e0d3a62

mahoneycm mentioned this pull request Jun 7, 2023

USWDS-Site - Changelog: Add entry for Card fix border calc errors [USWDS#5325] uswds/uswds-site#2127

Merged

mahoneycm requested review from mejiaj and amyleadem June 7, 2023 18:45

mahoneycm marked this pull request as ready for review June 7, 2023 18:45

amyleadem self-assigned this Jun 27, 2023

mejiaj suggested changes Jul 10, 2023

View reviewed changes

mahoneycm added 4 commits July 21, 2023 15:56

Replace mixins that pass in -card-border-width with native options

37ce24d

Remove unused header--exdent style definitions

db12e14

Fix margin-top calculation to match new * -1 pattern

7b3d74c

Necessary for string tokens to become negative

Restore header exdent variants

35a9e8a

mahoneycm requested a review from mejiaj July 25, 2023 15:04

amyleadem approved these changes Jul 28, 2023

View reviewed changes

mejiaj approved these changes Aug 2, 2023

View reviewed changes

mejiaj requested a review from thisisdano August 2, 2023 16:34

mahoneycm mentioned this pull request Aug 3, 2023

USWDS - Card: Fix header first media overlap #5423

Merged

2 tasks

amyleadem mentioned this pull request Aug 8, 2023

USWDS-Site: Documentation updates for release 3.5.1 uswds/uswds-site#2223

Closed

thisisdano approved these changes Aug 11, 2023

View reviewed changes

thisisdano merged commit 9e86c47 into develop Aug 11, 2023

thisisdano deleted the cm-card-border-width-0 branch August 11, 2023 16:34

thisisdano mentioned this pull request Aug 18, 2023

Release 3.6.0 #5454

Merged

mejiaj mentioned this pull request Sep 28, 2023

USWDS - Bug: Error in card padding setting #5515

Closed

sennhann29 mentioned this pull request Jul 26, 2024

[Snyk] Upgrade @uswds/uswds from 3.4.1 to 3.8.1 sennhann29/digitalgov.gov#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

USWDS - Card: Fix `$theme-card-border-width` calculation errors #5325

USWDS - Card: Fix `$theme-card-border-width` calculation errors #5325

Uh oh!

mahoneycm commented Jun 7, 2023 •

edited by amyleadem

Loading

Uh oh!

amyleadem commented Jun 27, 2023

Uh oh!

mahoneycm commented Jul 5, 2023

Uh oh!

mejiaj left a comment •

edited

Loading

Uh oh!

mahoneycm commented Jul 20, 2023

Uh oh!

mejiaj commented Jul 20, 2023

Uh oh!

mahoneycm commented Jul 20, 2023

Uh oh!

mahoneycm commented Jul 25, 2023

Uh oh!

amyleadem left a comment

Uh oh!

mejiaj left a comment •

edited

Loading

Uh oh!

mejiaj Aug 2, 2023

Uh oh!

mejiaj Aug 2, 2023 •

edited

Loading

Uh oh!

mahoneycm Aug 2, 2023

Uh oh!

mejiaj Aug 3, 2023

Uh oh!

thisisdano left a comment

Uh oh!

Uh oh!

USWDS - Card: Fix $theme-card-border-width calculation errors #5325

USWDS - Card: Fix $theme-card-border-width calculation errors #5325

Uh oh!

Conversation

mahoneycm commented Jun 7, 2023 • edited by amyleadem Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking change

Related issue

Related pull requests

Preview link

Problem statement

Setting $theme-card-border-width to zero

Setting $theme-card-border-width to a string token

Solution

Testing and review

Testing Checklist

Additional info

Footnotes

Uh oh!

amyleadem commented Jun 27, 2023

Uh oh!

mahoneycm commented Jul 5, 2023

Uh oh!

mejiaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm commented Jul 20, 2023

Uh oh!

mejiaj commented Jul 20, 2023

Uh oh!

mahoneycm commented Jul 20, 2023

Uh oh!

mahoneycm commented Jul 25, 2023

Uh oh!

amyleadem left a comment

Choose a reason for hiding this comment

Uh oh!

mejiaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Sandbox testing

Footnotes

Uh oh!

mejiaj Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

mejiaj Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

mejiaj Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

thisisdano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

USWDS - Card: Fix `$theme-card-border-width` calculation errors #5325

USWDS - Card: Fix `$theme-card-border-width` calculation errors #5325

mahoneycm commented Jun 7, 2023 •

edited by amyleadem

Loading

Setting `$theme-card-border-width` to zero

Setting `$theme-card-border-width` to a string token

mejiaj left a comment •

edited

Loading

mejiaj left a comment •

edited

Loading

mejiaj Aug 2, 2023 •

edited

Loading