-
Notifications
You must be signed in to change notification settings - Fork 1.1k
USWDS - Settings: Fix math errors with $theme-site-margins-width #5582
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
Conversation
There was a problem hiding this 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.
padding-left: units($theme-site-margins-width) * 2; | ||
padding-right: units($theme-site-margins-width) * 2; |
There was a problem hiding this comment.
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
padding-left: units($theme-site-margins-width) * 2; | |
padding-right: units($theme-site-margins-width) * 2; | |
padding-inline: units($theme-site-margins-width) * 2; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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: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
padding
property rather than theu-padding
mixin allows the system to successfully do the required calculations.calc()
enables the math to work when there are mixed units (For example, when$theme-site-margins-width
is set to1px
).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
:$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.