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

Feat(variables-scss): Variables-scss exporter can export new tokens structure #DS-1435 #1592

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

curdaj
Copy link
Contributor

@curdaj curdaj commented Aug 26, 2024

Description

Additional context

Issue reference

Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 2274469
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/66d6bbdb2998e4000875c43f
😎 Deploy Preview https://deploy-preview-1592--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (🟢 up 1 from production)
Accessibility: 93 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 2274469
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/66d6bbdb16426c0008494ed6

@github-actions github-actions bot added the feature New feature or request label Aug 26, 2024
@curdaj curdaj changed the title Feat(exporters): Variable css exporter can export new tokens #DS-1435 🚧 Feat(exporters): Variable css exporter can export new tokens #DS-1435 Aug 26, 2024
@curdaj curdaj changed the title 🚧 Feat(exporters): Variable css exporter can export new tokens #DS-1435 🚧 Feat(exporters): Variables-css exporter can export new tokens structure #DS-1435 Aug 26, 2024
Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

The holy trinity of the filter, map and reduce is your best friend :-)

exporters/variables-scss/.eslintrc.js Outdated Show resolved Hide resolved
exporters/variables-scss/src/index.ts Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Aug 27, 2024

Coverage Status

coverage: 78.449% (-0.7%) from 79.191%
when pulling 2274469 on feat/ds-1435-exporter-can-export-new-tokens
into cdc61cf on main.

@curdaj curdaj changed the title 🚧 Feat(exporters): Variables-css exporter can export new tokens structure #DS-1435 Feat(exporters): Variables-css exporter can export new tokens structure #DS-1435 Aug 27, 2024
@curdaj curdaj marked this pull request as ready for review August 27, 2024 11:46
@curdaj curdaj requested review from crishpeen, pavelklibani and a team as code owners August 27, 2024 11:46
@curdaj curdaj changed the title Feat(exporters): Variables-css exporter can export new tokens structure #DS-1435 Feat(variables-scss): Variables-scss exporter can export new tokens structure #DS-1435 Aug 27, 2024
@literat
Copy link
Collaborator

literat commented Aug 28, 2024

I have tried to run this for the first time and it is looking great 👍

But there are two things that need to be fixed:

  • missing - in SCSS variable:
  $container padding:
  $border radius: 
  $spacing systems:
  • using tabs instead of spaces (non-blocking right now)

@literat
Copy link
Collaborator

literat commented Aug 28, 2024

You are also using the content directory for all code. Is this enclosing necessary? Can be the formatters, generators and helpers placed right under the src directory?

@curdaj
Copy link
Contributor Author

curdaj commented Aug 29, 2024

  • missing - in SCSS variable:
  $container padding:
  $border radius: 
  $spacing systems:

Hmm, shouldn't this be spaces, radii?

Its probably outdated now

@curdaj
Copy link
Contributor Author

curdaj commented Aug 30, 2024

I have tried to run this for the first time and it is looking great 👍

But there are two things that need to be fixed:

  • missing - in SCSS variable:
  $container padding:
  $border radius: 
  $spacing systems:
  • using tabs instead of spaces (non-blocking right now)

probably from CR to draft - outdated

@literat
Copy link
Collaborator

literat commented Aug 30, 2024

Run yarn format:check
Checking formatting...
[warn] exporters/variables-scss/generated/exporter.cjs
[warn] Code style issues found in the above file. Run Prettier with --write to fix.

I think the generated directory should be ignored by Prettier. 🤔

@literat
Copy link
Collaborator

literat commented Aug 30, 2024

Also, tests are failing with:

src/content/formatters/__tests__/cssFormatter.test.ts
  ● formatCSS › should correctly format CSS string

    expect(received).toBe(expected) // Object.is equality

    - Expected  - 3
    + Received  + 3

      $my-var: (
    -     color: #000,
    +         color: #000,
    -     background: #fff,
    +         background: #fff,
    -     border: 1px solid #000,
    +         border: 1px solid #000,
      ) !default;
      ↵

      12 | describe('formatCSS', () => {
      13 |   it('should correctly format CSS string', () => {
    > 14 |     expect(formatCSS(mockedUnformattedCSS)).toBe(mockedFormattedCSS);
         |                                             ^
      15 |   });
      16 | });
      17 |

      at Object.<anonymous> (src/content/formatters/__tests__/cssFormatter.test.ts:14:

@curdaj
Copy link
Contributor Author

curdaj commented Sep 2, 2024

Member

I don't understand this. It used to work normally for me and now it doesn't work again.

@curdaj
Copy link
Contributor Author

curdaj commented Sep 2, 2024

Member

I don't understand this. It used to work normally for me and now it doesn't work again.

It seems to me that something formatted the "unformatted.scss" and that's why it was failing. I'm not sure if prittier won't do something there.

@curdaj
Copy link
Contributor Author

curdaj commented Sep 2, 2024

Run yarn format:check
Checking formatting...
[warn] exporters/variables-scss/generated/exporter.cjs
[warn] Code style issues found in the above file. Run Prettier with --write to fix.

I think the generated directory should be ignored by Prettier. 🤔

It started failing since I changed it to cjs according to CR

@curdaj curdaj force-pushed the feat/ds-1435-exporter-can-export-new-tokens branch from 86ae506 to 9f0108e Compare September 2, 2024 08:24
@curdaj
Copy link
Contributor Author

curdaj commented Sep 2, 2024

Run yarn format:check
Checking formatting...
[warn] exporters/variables-scss/generated/exporter.cjs
[warn] Code style issues found in the above file. Run Prettier with --write to fix.

I think the generated directory should be ignored by Prettier. 🤔

It started failing since I changed it to cjs according to CR

EDIT: solved. I added this file to prettierignore

@curdaj
Copy link
Contributor Author

curdaj commented Sep 2, 2024

Member

I don't understand this. It used to work normally for me and now it doesn't work again.

It seems to me that something formatted the "unformatted.scss" and that's why it was failing. I'm not sure if prittier won't do something there.

SOLVED - the problem was that prettier always formatted the example scss file before the commit. I added it to the prettierignore

Copy link
Contributor

@pavelklibani pavelklibani left a comment

Choose a reason for hiding this comment

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

Good job ❗

@curdaj curdaj force-pushed the feat/ds-1435-exporter-can-export-new-tokens branch from d193401 to f8cc678 Compare September 2, 2024 13:56
@curdaj curdaj force-pushed the feat/ds-1435-exporter-can-export-new-tokens branch from 4729a59 to 2274469 Compare September 3, 2024 07:33
@curdaj curdaj merged commit 59f8021 into main Sep 3, 2024
14 checks passed
@curdaj curdaj deleted the feat/ds-1435-exporter-can-export-new-tokens branch September 3, 2024 08:03
@@ -68,3 +68,9 @@ makefile

# GitHub
CODEOWNERS

# variable-scss exporter example mock test files
exporters/variables-scss/src/**/*.scss
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe better using __fixtures__ to ignore.

exporters/variables-scss/src/**/*.scss

# variable-scss exporter generated cjs
exporters/variables-scss/generated/**/*.cjs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can use just generated name in general.

JSON.stringify(
tokens.map((token) => ({
tokenType: token.tokenType,
// @ts-ignore-next-line
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing description for this comment why we are ignoring this. And same for the comment few lines lower.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants