-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add modern spacing utilities from design system #248
Conversation
Size Change: +2.66 kB (+3%) Total Size: 92.5 kB
ℹ️ View Unchanged
|
@@ -22,7 +22,7 @@ $CSS_BORDER_EDGE_CORNERS: ( | |||
$grid-columns: 6 !default; | |||
|
|||
$breakpoint-aliases: ( | |||
xs: $mq-tiny, | |||
// xs: $mq-tiny, |
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.
We don't use the xs breakpoint in any of our templates, so this can go away (and saves us a ton on CSS payload size).
28: 28px, | ||
40: 40px, | ||
60: 60px, | ||
// 80: 80px, |
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 leaving this commented out for now because we don't appear to use it anywhere, and adding it has a big impact on our CSS bundle size because of how the utilities are generated.
// 4: 60px, | ||
// 5: 96px, |
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've replaced all of the instances of [mp][trblxy]-[45]
with the -60
and -96
equivalents in our templates.
16: 16px, | ||
20: 20px, | ||
28: 28px, | ||
40: 40px, |
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.
what's the thinking for having different tokens for the same value, e.g. 40
/3
for 40px
? do we need to keep both to get around this safelist issue and maintain support across templates?
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.
It's really just for backwards compatibility: I didn't go all the way in replacing the -3
utilities in the templates, and there might be forms out there with classes hard-coded in HTML components that I don't want to break.
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.
got it, I'm seeing the -40
utilities in the deployment so I'll approve!
* bump v10.0.0 * Add modern spacing utilities from design system (#248) * fix(css): add design system spacing utilities * fix: replace 4+5 spacers with absolute values * BREAKING CHANGE: nix xs breakpoint, 4 + 5 spacers * fix: add 96px spacer * chore: rebuild test snapshots * fix: fix failing test * chore(deps): update browserslist db * fix(deps): upgrade jest * chore(deps): npm audit fix * chore(deps): upgrade jest-serializer-html * chore(npm): fix engines.node * chore(deps): install source-map to fix build issues * chore(test): update snapshots Co-authored-by: shawnbot <[email protected]>
Until we figure out what's up with the safelist not working in SFDigitalServices/sfgov#1462, we need to add these spacing utilities here to fix existing forms.
This one ended up being way more complicated than I'd hoped. Adding new CSS and then changing the templates fails the build because the snapshots need to be updated. But I couldn't figure out why my snapshots weren't matching in CI until I remembered that the tests run the built JS, so you have to build the JS before updating the snapshots:
Of course, this yielded a cryptic error:
I fixed this with
npm i -D source-map
, as suggested in mozilla/source-map#432.Heads up to @aekong on this too 😄