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

[WNMGDS-2935] Make Sass token file changes backwards compatible #3230

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

pwolfert
Copy link
Collaborator

Summary

Makes our Figma changes backwards compatible with respect to the sass var files. I had failed to adequately document the changes in #3100, which resulted in a failure to report the breaking change of a file-name change in our release notes. Luckily our integration tests caught it in one of the beta releases.

Note that instead of reproducing the same old contents of those files, they're just duplicates of the new file, and that's enough. It won't break anything if a product imports both of these files, because they just re-declare the same variables. It also has no effect on the final CSS bundle sizes because Sass variables are ephemeral.

How to test

Clear all your dist folders and rebuild. You should see the same old file names that we used to output.

@kim-cmsds, could you also test this directly in the downstream app that had the issue?

Checklist

  • Prefixed the PR title with the Jira ticket number as [WNMGDS-####] Title or [NO-TICKET] if this is unticketed work.
  • Selected appropriate Type (only one) label for this PR, if it is a breaking change, label should only be Type: Breaking
  • Selected appropriate Impacts, multiple can be selected.
  • Selected appropriate release milestone

@pwolfert pwolfert added Type: Fixed Indicates an unexpected problem or unintended behavior Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. labels Sep 16, 2024
@pwolfert pwolfert added this to the 11.0.0 milestone Sep 16, 2024
@tamara-corbalt
Copy link
Collaborator

tamara-corbalt commented Sep 16, 2024

It seems like the snapshot for tokenFilesToScssFiles needs to be updated.

 FAIL  design-system-tokens/src/css/translate.test.ts
  ● tokenFilesToScssFiles › matches snapshot

    expect(received).toMatchSnapshot()

    Snapshot name: `tokenFilesToScssFiles matches snapshot 1`

Otherwise, it's working for me locally—I followed the test instructions, and the dist folders contain the expected SASS files.

@kim-cmsds
Copy link
Collaborator

@pwolfert I'm seeing an error after trying this out locally. On this branch, I ran the yarn clean && yarn build commands within the design-system-tokens package. Then I ran npm install and manually copied the backwards compatible healthcare sass files (healthcare-component-tokens.scss and healthcare-tokens.scss) into the downstream project's node_modules/@cmsgov/ds-healthcare-gov/dist/scss folder. The error I saw when running npm run start in the downstream project folder is as follows:

8:24:05 AM [vite] Internal server error: [sass] Undefined variable.
    ╷
135 │ $font-weight-heading-5xl: $font-weight-bold;
    │                           ^^^^^^^^^^^^^^^^^
    ╵
  node_modules/@cmsgov/ds-healthcare-gov/dist/scss/healthcare-tokens.scss 135:27  @import
  src/index.scss 6:9                                                              root stylesheet
  Plugin: vite:css
  File: hra-tool/node_modules/@cmsgov/ds-healthcare-gov/dist/scss/healthcare-tokens.scss:135:27
  Error: Undefined variable.

@pwolfert
Copy link
Collaborator Author

pwolfert commented Sep 17, 2024

@kim-cmsds, you found a new bug! The order doesn't matter for CSS variables, but in Sass a variable can't be referenced before it's defined. That means my naive alphabetical default ordering isn't going to work. Good catch!

(Right now I put the system tokens first and then the theme tokens, but in this case there is a theme token referencing another theme token before it's defined.)

@pwolfert pwolfert merged commit 6038634 into main Sep 17, 2024
1 check passed
@pwolfert pwolfert deleted the pwolfert/replicate-old-sass-output branch September 17, 2024 20:21
jack-ryan-nava-pbc pushed a commit that referenced this pull request Sep 18, 2024
* Make our Figma changes backwards compatible with respect to the sass var files

* In the unit tests, acknowledge but don't snapshot the duplicated files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. Type: Fixed Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants