Remove SASS from @shopify/polaris #11677

jesstelford · 2024-03-01T00:58:40Z

Part of https://github.com/Shopify/polaris-internal/issues/1345

I've tophatted the storybook
I've tophatted this on staging staging-sxpt

Reviewing notes

The majority of changes in this PR are file renames (.scss -> .css & their imports), which makes it hard to review the changes :(

To view the config related changes, use this command:

git diff main..HEAD -- ':!polaris-react/src/*' ':!polaris-react/postcss-mixins/*' ':!*.scss'

To see what the diff of the final generated .css is, read on...

Diff details

As well as Tophatting Storybook & web, another way to confirm nothing has broken / changed is comparing the built styles.css file agains the current version.

The output is not identical, but we didn't expect it to be (eg; moving to CSS Custom Properties). We developed a script to normalize the output between the two built .css files (one from main, the other from this remove-sass branch) which minimizes the diff to what's actually changed.

For the full diff with inline comments on what's changed and why, see https://github.com/Shopify/polaris/pull/11718/files

Performance

tl;dr: Building is now 6 seconds slower with postcss.

Performance test ran with hyperfine:

❯ hyperfine --parameter-list branch f9d5f25b9,f6ba2b2a8 --setup='git checkout {branch} && rm -rf node_modules */node_modules\(N\) && . ~/.nvm/nvm.sh && nvm use && yarn --prefer-offline' --prepare='rm -rf polaris-for-vscode/dist polaris-icons/dist polaris-migrator/dist polaris-react/build polaris-react/build-internal polaris-tokens/dist polaris.shopify.com/.next' --runs 5 --warmup 1 '. ~/.nvm/nvm.sh && nvm use && yarn build --filter="@shopify/polaris" --force --no-cache'

Benchmark 1: (SASS)
  Time (mean ± σ):     27.781 s ±  0.641 s    [User: 40.161 s, System: 5.228 s]
  Range (min … max):   27.087 s … 28.579 s    5 runs

Benchmark 2: (postcss)
  Time (mean ± σ):     33.619 s ±  1.117 s    [User: 46.152 s, System: 6.066 s]
  Range (min … max):   32.103 s … 34.852 s    5 runs

Summary
  SASS ran 1.21 ± 0.05 times faster than postcss

jesstelford · 2024-03-12T07:46:33Z

/snapit

github-actions · 2024-03-12T07:49:16Z

🫰✨ Thanks @jesstelford! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/[email protected]

yarn add @shopify/[email protected]

chloerice

Man oh man, this was a non-trivial load of work to accomplish, well done y'all. Code looks good. In order to feel confident in shipping this, it needs to be on a web staging environment. I'll draft a PR and cross-link to this one so folks can tophat on any store.

.changeset/slimy-kings-train.md

chloerice · 2024-03-12T19:05:30Z

.stylelintrc.js

+          /.*/,
+          {
+            message:
+              'Comments in polaris-react/postcss-mixins/*.css cause postcss-mixins to error. To disable lint rules, add them to styleslintrc.js overrides.',


chloerice · 2024-03-12T19:26:12Z

stylelint-polaris/plugins/coverage/index.js

+
+(e.g., Unexpected named color "blue" - Please use a Polaris color token Stylelint(polaris/colors/color-named)")
+*/
+const stylelintPolarisCoverageOptions = {


Can/should the rule's configuration move into its own file instead of into the rule's plugin file?

Yeah, definitely can - I just wholesale copy+pasted it from the stylelint-polaris index file 🤷

chloerice · 2024-03-12T19:45:28Z

🫰✨ Thanks @jesstelford! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:
yarn add @shopify/[email protected]
yarn add @shopify/[email protected]

Does something need to change in the publish action along with rest of these changes? @shopify/polaris is not getting published with the other snapshots in spite of the majority of changes being in that package 👀 Or maybe it's just because there's no changeset targeting it 🤔 Pushed up a change log draft for @shopify/polaris to hopefully unblock getting this branch onto staging in web.

alex-page · 2024-03-12T19:52:42Z

Or maybe it's just because there's no changeset targeting it

Yeah you need a changeset otherwise it doesn't think this PR has any changes and just snapshots main

chloerice · 2024-03-12T19:55:20Z

/snapit

github-actions · 2024-03-12T19:58:49Z

🫰✨ Thanks @chloerice! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/[email protected]

yarn add @shopify/[email protected]

yarn add @shopify/[email protected]

jesstelford · 2024-03-12T23:11:21Z

Thanks for getting the snapshot release sorted @chloerice!

…th backwards compat for SASS

… Polaris

… folder

…ecks

jesstelford · 2024-03-14T01:18:11Z

/snapit

github-actions · 2024-03-14T01:21:35Z

🫰✨ Thanks @jesstelford! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

yarn add @shopify/[email protected]

alex-page · 2024-03-14T01:36:54Z

polaris-react/playground/DetailsPage.module.css

+  // stylelint-disable-next-line -- polaris custom global property
+  height: var(--pg-top-bar-height);


I haven't seen --pg- before I couldn't quicky find it in this PR but where are these defined?

All the --pg- custom properties are defined in polaris-react/src/components/AppProvider/global.css (previously .scss) - they're "global" variables (hence the g) that used to be global SASS variables and are used across components.

--pg- has been added to the linter as a disallowed custom property prefix (and isn't used anywhere so it's a non-breaking change to the linter).

Thanks @jesstelford makes sense to me. I didn't like these as scss variables and I also don't like it now but it makes sense until we refactor some of these global layouts and fix what UI needs these values.

Could we replace any of these with size tokens?

We definitely could replace them with hardcoded values in some cases. But then we'd lose the semantic naming and the relationship between where those values are used.

For example, --pg-layout-width-inner-spacing-base & its uses could be replaced with --p-size-400, but even though it's only used in one place, the loss of that semantic information would be a serious regression in my opinion.

…11733) Following on from [the conversion of SASS -> CSS](#11677), this PR converts the SASS style inline comments to CSS style multiline comments. Since this was the only reason we were using `postcss-scss` parser, it too has been removed. --- I used this script to achieve the conversion: ```sh #!/bin/bash # Write a temporary postcss config for postcss to work cat <<EOF > postcss.config.js const spaceAtEndRegex = /\s$/; const postCssInlineToMultilineComment = { postcssPlugin: 'postcss-inline-to-multiline-comment', Comment(node) { if (node.raws.inline && (!node.raws.right || !spaceAtEndRegex.test(node.raws.right))) { if (!node.raws.right) { node.raws.right = ''; } node.raws.right += ' '; } }, } module.exports = { // Need to understand SCSS inline comment syntax parser: 'postcss-scss', plugins: [ postCssInlineToMultilineComment, ], }; EOF npx postcss-cli --yes postcss-cli -r --no-map --config="`pwd`" polaris-react/**/*.css rm -f postcss.config.js # Clean up temporary files ``` Then I confirmed it was successful with this script: ```sh if [ -z "$1" ]; then echo "A filename is required:\n> $0 ./no-inline.css"; exit 1; fi; # Write a temporary postcss config for cssnano to work cat <<EOF > postcss.config.js module.exports = { plugins: [ require('cssnano')({ preset: 'lite', }), ], }; EOF yarn workspace @shopify/polaris add cssnano-preset-lite yarn yarn build --filter="@shopify/polaris" cp polaris-react/build/esm/styles.css "$1" # Move out of ignored directories yarn prettier -w "$1" # Pre-format to normalize whitespace npx postcss-cli --yes postcss-cli -r --no-map --config="`pwd`" "$1" # cssnano removes more whitespace prettier doesn't yarn prettier -w "$1" # Format again to get back into un-minified version rm postcss.config.js # Clean up temporary files yarn workspace @shopify/polaris remove cssnano-preset-lite ``` Run like this: ```sh git checkout multiline-comments ./go.sh ./no-inline.css git checkout main ./go.sh ./inline.css git checkout - git diff --histogram --no-index inline.css no-inline.css # Should be an empty diff ```

@jesstelford

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/[email protected] ### Minor Changes - [#11677](#11677) [`f6ba2b2a8`](f6ba2b2) Thanks [@jesstelford](https://github.com/jesstelford)! - Migrated @shopify/polaris from SASS to CSS using postcss plugins - [#11723](#11723) [`4699bb229`](4699bb2) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated `BulkActions` to only show actions when selectMode is `true` - [#11727](#11727) [`c3ba6ae1b`](c3ba6ae) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Removed the responsive logic that disabled the Card bevel on mobile. Removing this until we are ready to rollout bevel changes across all components. ### Patch Changes - [#11757](#11757) [`e0ae9565c`](e0ae956) Thanks [@sophschneider](https://github.com/sophschneider)! - Added dynamicTopBarAndReframe feature flag type - [#11733](#11733) [`9c24a465c`](9c24a46) Thanks [@jesstelford](https://github.com/jesstelford)! - Convert SASS-style inline comments to CSS-style multiline comments. - [#11724](#11724) [`1543246b7`](1543246) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Updated responsive styles for `Text` component - [#11765](#11765) [`42c298ea7`](42c298e) Thanks [@jesstelford](https://github.com/jesstelford)! - Fix build performance regression from using postcss-mixins. - [#11725](#11725) [`3e011e3b6`](3e011e3) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed a bug where iOS 16 font patch wasn't added for mobile app web views - [#11763](#11763) [`e7ab4a8f5`](e7ab4a8) Thanks [@sydturn](https://github.com/sydturn)! - Fixed `IndexTable.Row` `onClick` not being called when `selectable` is `false` - [#11745](#11745) [`831a683a2`](831a683) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed bug in math.ts for popover with position cover - [#11735](#11735) [`6d8ef8c99`](6d8ef8c) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Used `Text` component to apply text styles for `Button` - [#11592](#11592) [`ad6315845`](ad63158) Thanks [@SMAKSS](https://github.com/SMAKSS)! - Passed missing `id` prop to the root element of `BlockStack` ## @shopify/[email protected] ### Minor Changes - [#11677](#11677) [`f6ba2b2a8`](f6ba2b2) Thanks [@jesstelford](https://github.com/jesstelford)! - Add `--pg-` as a disallowed CSS Custom Property prefix (these are "global" Custom Properties used within @shopify/polaris-react). ## @shopify/[email protected] ### Patch Changes - [#11754](#11754) [`f57db81df`](f57db81) Thanks [@jesstelford](https://github.com/jesstelford)! - Move migrations to v14 since the node v20 requirement will be the only change in v13 - Updated dependencies \[[`f6ba2b2a8`](f6ba2b2)]: - @shopify/[email protected] ## [email protected] ### Minor Changes - [#11720](#11720) [`97184dc80`](97184dc) Thanks [@sarahill](https://github.com/sarahill)! - Added common action guidance. - [#11690](#11690) [`c78f125c7`](c78f125) Thanks [@heyjoethomas](https://github.com/heyjoethomas)! - Updates images for icon design guidance ### Patch Changes - [#11747](#11747) [`5cff96245`](5cff962) Thanks [@sarahill](https://github.com/sarahill)! - Updated card layout patterns to match common action patterns. - Updated dependencies \[[`e0ae9565c`](e0ae956), [`9c24a465c`](9c24a46), [`f6ba2b2a8`](f6ba2b2), [`4699bb229`](4699bb2), [`c3ba6ae1b`](c3ba6ae), [`1543246b7`](1543246), [`42c298ea7`](42c298e), [`3e011e3b6`](3e011e3), [`e7ab4a8f5`](e7ab4a8), [`831a683a2`](831a683), [`6d8ef8c99`](6d8ef8c), [`ad6315845`](ad63158)]: - @shopify/[email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Part of https://github.com/Shopify/polaris-internal/issues/1345 - [x] I've tophatted [the storybook](https://5d559397bae39100201eedc1-bldfeadhvc.chromatic.com/) - [x] I've tophatted this [on staging](https://web.docs.shopify.io/docs/guides/staging#testing-your-code-in-staging) `staging-sxpt` --- ## Reviewing notes The majority of changes in this PR are file renames (`.scss` -> `.css` & their imports), which makes it hard to review the changes :( To view the config related changes, use this command: ```bash git diff main..HEAD -- ':!polaris-react/src/*' ':!polaris-react/postcss-mixins/*' ':!*.scss' ``` To see what the diff of the final generated .css is, read on... ### Diff details As well as Tophatting Storybook & web, another way to confirm nothing has broken / changed is comparing the built `styles.css` file agains [the current version](https://unpkg.com/browse/@shopify/polaris/build/esm/styles.css). The output is not identical, but we didn't expect it to be (eg; moving to CSS Custom Properties). We developed [a script](https://github.com/Shopify/polaris/blob/remove-sass-compiled-css/go.sh) to normalize the output between the two built `.css` files (one from `main`, the other from this `remove-sass` branch) which minimizes the diff to what's actually changed. For the full diff with inline comments on what's changed and why, see https://github.com/Shopify/polaris/pull/11718/files --------- Co-authored-by: Charles Lee <[email protected]>

lgriffee added the #gsd:38846 Admin Quality Improvements (Q1 2024) label Mar 1, 2024

jesstelford force-pushed the remove-sass branch 2 times, most recently from 126d2e6 to aea629a Compare March 5, 2024 23:46

gwyneplaine marked this pull request as ready for review March 7, 2024 04:43

jesstelford force-pushed the remove-sass branch 3 times, most recently from 26a8297 to 7705d8f Compare March 12, 2024 03:34

jesstelford mentioned this pull request Mar 12, 2024

[DO NOT MERGE] Remove SASS CSS diff #11718

Closed

chloerice requested a review from a team March 12, 2024 18:34

chloerice reviewed Mar 12, 2024

View reviewed changes

jesstelford force-pushed the remove-sass branch from 4bca364 to 54fa756 Compare March 14, 2024 00:46

jesstelford added 12 commits March 14, 2024 11:55

SASS variables converted to postcss-advanced-variables

dc74b5d

Patch postcss-advanced-variables to v4.0.0 (commit 085a559)

7f341fa

Add postcss-advanced-variables

3132cc6

Add postcss-scss for inline comment support

b31d049

Convert @include focus-ring to how postcss-advanced-variables expects

168aa17

Make postcss happy by migrating away from SCSS syntax

3048869

Remove unused node-sass dependency

a636a75

Add postcss-nesting to support CSS-spec compliant selector nesting wi…

ff46c95

…th backwards compat for SASS

Strip comments from postcss output

c6fc994

Remove unnecessary quotes on mixin params

04fa1c3

Fix math in for loop to work with modern CSS

a6d38e6

Move SASS mixin & variable definitions into global CSS

78b9c3f

gwyneplaine and others added 16 commits March 14, 2024 12:10

Ensure stylelint-polaris errors on --pg custom property usages within…

efed252

… Polaris

stylelint disables for --pg custom property usages

2fcdc69

Remove comments which break postcss-mixins and disable rules for that…

3f91fb7

… folder

Resolve invalid flex calculation in Layout.module.css

bd7ff3c

Fix custom-property invocations in Navigation.module.css

c8afeeb

Fix comma separated browser selector bug in SingleThumb.module.css

3221169

Fix lint errors in css

ee000f7

Remove erroneous safe-area-for call

9a029ba

Reorder mixin track selectors to match old order

6f0bb39

Ensure charset is always set

f1c6451

Fix typo

f6aaec9

Cleanup / fix Navigation variable names

8506bdd

Re-order media query so custom-queries works for print styles

11cb785

Add duplicate mixin to simplify some refactors

47609db

Undo earlier refactor that moved a mixin to client side breakpoint ch…

cfbcc5b

…ecks

Disable polaris/coverage for mixins

9f11ee6

jesstelford force-pushed the remove-sass branch from 54fa756 to 9f11ee6 Compare March 14, 2024 01:11

Add changeset

b58b031

alex-page reviewed Mar 14, 2024

View reviewed changes

alex-page approved these changes Mar 14, 2024

View reviewed changes

Add changeset for @shopify/polaris-react

b3abf3a

jesstelford merged commit f6ba2b2 into main Mar 14, 2024
12 checks passed

jesstelford deleted the remove-sass branch March 14, 2024 03:11

shopify-github-actions-access bot mentioned this pull request Mar 13, 2024

Version Packages #11719

Merged

jesstelford mentioned this pull request Mar 14, 2024

Convert SASS-style inline comments to CSS-style multiline comments. #11733

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SASS from @shopify/polaris #11677

Remove SASS from @shopify/polaris #11677

jesstelford commented Mar 1, 2024 •

edited

Loading

jesstelford commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

chloerice left a comment

chloerice Mar 12, 2024

chloerice Mar 12, 2024

jesstelford Mar 12, 2024

chloerice commented Mar 12, 2024 •

edited

Loading

alex-page commented Mar 12, 2024 •

edited

Loading

chloerice commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

jesstelford commented Mar 12, 2024

jesstelford commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

alex-page Mar 14, 2024

jesstelford Mar 14, 2024

alex-page Mar 14, 2024 •

edited

Loading

jesstelford Mar 14, 2024

		// stylelint-disable-next-line -- polaris custom global property
		height: var(--pg-top-bar-height);

Remove SASS from @shopify/polaris #11677

Remove SASS from @shopify/polaris #11677

Conversation

jesstelford commented Mar 1, 2024 • edited Loading

Reviewing notes

Diff details

Performance

jesstelford commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

chloerice left a comment

Choose a reason for hiding this comment

chloerice Mar 12, 2024

Choose a reason for hiding this comment

chloerice Mar 12, 2024

Choose a reason for hiding this comment

jesstelford Mar 12, 2024

Choose a reason for hiding this comment

chloerice commented Mar 12, 2024 • edited Loading

alex-page commented Mar 12, 2024 • edited Loading

chloerice commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

jesstelford commented Mar 12, 2024

jesstelford commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

alex-page Mar 14, 2024

Choose a reason for hiding this comment

jesstelford Mar 14, 2024

Choose a reason for hiding this comment

alex-page Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

jesstelford Mar 14, 2024

Choose a reason for hiding this comment

jesstelford commented Mar 1, 2024 •

edited

Loading

chloerice commented Mar 12, 2024 •

edited

Loading

alex-page commented Mar 12, 2024 •

edited

Loading

alex-page Mar 14, 2024 •

edited

Loading