Skip to content

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Oct 26, 2023

Summary

Fixed a bug that prevented $theme-site-margins-width from accepting expected token values.

Breaking change

This is not a breaking change.

Related issue

Closes #5559

Related pull requests

Changelog PR

Preview link

Alert component

Problem statement

When setting custom values for $theme-site-margins-width, some values that the system says are acceptable cause an error.

For example,$theme-site-margins-width: 6 results in the following error:

SassError: "`12` is not a valid `padding` token. You should correct this. Standard `padding` tokens:  1px, 2px, 05, 1, 105, 2, 205, 3, 4, 5, 6, 7, 8, 9, 10, 15, 0"

19 │     padding-left: get-uswds-value("padding", $value...) #{$important};
    

This happens because the u-padding mixins accept only known padding units. The current calculations perform multiplication on the setting value before running it through the mixin, which means that the mixins are not always receiving acceptable unit values. For example, when $theme-site-margins-width is defined with 6, the mixin receives the value of 12, which is not a mapped unit value.

Solution

  1. Defining the padding with the CSS padding property rather than the u-padding mixin allows the system to successfully do the required calculations.
  2. Wrapping calculations inside calc() enables the math to work when there are mixed units (For example, when $theme-site-margins-width is set to 1px).

Warning
This PR uncovered that the alert component has some alignment issues when the value for $theme-site-margins-width is adjusted. It can cause text to overlap or become misaligned with the other components. I have opened issue #5583 to address this.

Testing and review

To test the alignment with default value for $theme-site-margins-width:
Open the alert component and confirm no visual regressions in alignment.

To test custom values for $theme-site-margins-width:

  1. Check out this branch in a local build
  2. Customize the value of $theme-site-margins-width to any of the following values: 1px, 2px, 05, 1, 105, 2, 205, 3, 4, 5, 6, 7, 8, 9, 10, 15, 0.
  3. Confirm that the Sass compiles without error
  4. Confirm the visual presentation adjusts as expected.
    • ⚠️ Note that the alert component will not adjust as expected. See the warning above for more details.

@amyleadem amyleadem marked this pull request as ready for review October 26, 2023 22:18
@amyleadem amyleadem requested review from mejiaj and mahoneycm October 26, 2023 22:18
@amyleadem amyleadem changed the title Fix math errors with $theme-site-margins-width USWDS - Settings: Fix math errors with $theme-site-margins-width Oct 26, 2023
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.

Lgtm! Left one small optional recommendation to keep the padding-x to a single line.

Comment on lines +93 to +94
padding-left: units($theme-site-margins-width) * 2;
padding-right: units($theme-site-margins-width) * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional

We could replicate the u-padding-x functionality with the native padding-inline property

Suggested change
padding-left: units($theme-site-margins-width) * 2;
padding-right: units($theme-site-margins-width) * 2;
padding-inline: units($theme-site-margins-width) * 2;
Screenshots

padding-left/right

image

padding-inline

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I avoided padding-inline here because it felt like it had meaning beyond what we intend at this time. I like the idea of moving towards padding-inline/padding-block in the future to better accommodate different language types, but in my mind it would require a comprehensive effort where we write styles for both block and inline. I might be overthinking it though! Curious to hear thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a valid point! Not opposed to leaving it as is since it is more specifically defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to now know about these two new properties

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it's the same value on each side, which at least makes it equivalent in LTR or RTL reading directions....

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.

LGTM

@thisisdano thisisdano merged commit 64e7dec into develop Nov 2, 2023
@thisisdano thisisdano deleted the al-theme-site-margins-fix branch November 2, 2023 16:50
@thisisdano thisisdano mentioned this pull request Nov 9, 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: theme-site-margin-width unit won't compile
3 participants