Skip to content
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

Avoid unit math on settings until after configured unit resolved #5076

Merged
merged 2 commits into from Feb 28, 2023

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Dec 21, 2022

Summary

Attempts to fix #5075 by avoiding math on configurable settings values which could result in an invalid unit token. The effect should be the same, but ensures that the configurable unit value is resolved before attempting any math on it.

Related issue

Fixes #5075

Problem statement

Developers should be able to configure settings to any valid unit and the build should pass.

Solution

Resolves configured unit value before performing math on it.

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.

Thank you! I was able to confirm the bug and the fix.

I also looked for other instances where we might be doing math inside of units, but didn't find any.

@mejiaj mejiaj modified the milestone: uswds 3.4.0 Jan 11, 2023
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

@aduth Thanks for fixing this!
I was able to confirm the issue and that the updates here fix the problem. I also confirmed that there were no visual discrepancies in the affected areas for banner and alert.

I did notice one more instance of calculations happening inside units() in usa-banner: https://github.com/uswds/uswds/blob/develop/packages/usa-banner/src/styles/_usa-banner.scss#L187

Would you be able to update that as well?
Let me know if you have questions. Thanks again!

@aduth
Copy link
Contributor Author

aduth commented Jan 19, 2023

Hi @amyleadem , in that case is the variable $size-touch-target meant to be configurable by a theme? I was under the impression it's a hard-coded value which wouldn't be prone to the same sorts of conflicts.

$size-touch-target: 6; // 48px to meet WCAG minimum of 44px

That being said, even if it's not configurable, I could see it being helpful to update it anyways for consistency. I'll push those changes shortly.

@aduth
Copy link
Contributor Author

aduth commented Jan 19, 2023

Updated the banner unit math in 3d830f7 .

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Thanks for the quick response on this @aduth! Looks good to me.

Copy link
Member

@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.

Thank you!

@thisisdano thisisdano merged commit 756512d into uswds:develop Feb 28, 2023
@thisisdano thisisdano mentioned this pull request Mar 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: Error when configuring spacing values to valid spacing tokens
4 participants