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

Add modern spacing utilities from design system #248

Merged
merged 13 commits into from
Jan 4, 2023

Conversation

shawnbot
Copy link
Member

@shawnbot shawnbot commented Jan 3, 2023

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:

npm run build-js
npx jest -u

Of course, this yielded a cryptic error:

> [email protected] build-js
> rollup -c


src/standalone.js → dist/formio-sfds.standalone.js...
[!] (plugin postcss) Error: You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer
dist/formio-sfds.css
Error: You must provide the URL of lib/mappings.wasm by calling SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) before using SourceMapConsumer
    at readWasm (/Users/shawn.allen/sfds/formio-sfds/node_modules/@csstools/postcss-sass/node_modules/source-map/lib/read-wasm.js:8:13)
    at wasm (/Users/shawn.allen/sfds/formio-sfds/node_modules/@csstools/postcss-sass/node_modules/source-map/lib/wasm.js:25:16)
    at /Users/shawn.allen/sfds/formio-sfds/node_modules/@csstools/postcss-sass/node_modules/source-map/lib/source-map-consumer.js:264:14
    at async Promise.all (index 0)

I fixed this with npm i -D source-map, as suggested in mozilla/source-map#432.

Heads up to @aekong on this too 😄

@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 3, 2023 18:40 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Size Change: +2.66 kB (+3%)

Total Size: 92.5 kB

Filename Size Change
dist/formio-sfds.css 15.1 kB +1.29 kB (+9%) 🔍
dist/formio-sfds.standalone.js 70.8 kB +1.37 kB (+2%)
ℹ️ View Unchanged
Filename Size
dist/portal.js 6.65 kB

compressed-size-action

@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:08 Inactive
@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:08 Inactive
src/scss/_tokens.scss Outdated Show resolved Hide resolved
@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:11 Inactive
@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:24 Inactive
@@ -22,7 +22,7 @@ $CSS_BORDER_EDGE_CORNERS: (
$grid-columns: 6 !default;

$breakpoint-aliases: (
xs: $mq-tiny,
// xs: $mq-tiny,
Copy link
Member Author

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,
Copy link
Member Author

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.

Comment on lines +37 to +38
// 4: 60px,
// 5: 96px,
Copy link
Member Author

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.

@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:32 Inactive
@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:40 Inactive
@shawnbot shawnbot temporarily deployed to formio-sfds-pr-248 January 4, 2023 00:55 Inactive
@shawnbot shawnbot marked this pull request as draft January 4, 2023 00:55
@shawnbot shawnbot marked this pull request as ready for review January 4, 2023 01:00
@shawnbot shawnbot requested a review from joni-f-g January 4, 2023 01:01
@shawnbot shawnbot mentioned this pull request Jan 4, 2023
2 tasks
@shawnbot shawnbot changed the base branch from main to release-10.0.0 January 4, 2023 01:10
16: 16px,
20: 20px,
28: 28px,
40: 40px,
Copy link

@joni-f-g joni-f-g Jan 4, 2023

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?

Copy link
Member Author

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.

Copy link

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!

@shawnbot shawnbot merged commit 5727a94 into release-10.0.0 Jan 4, 2023
@shawnbot shawnbot deleted the fix/spacing-utils branch January 4, 2023 17:50
shawnbot added a commit that referenced this pull request Jan 4, 2023
* 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]>
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.

2 participants