-
Notifications
You must be signed in to change notification settings - Fork 1
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): Introduce export to javascript #DS-1437 #1656
Conversation
✅ Deploy Preview for spirit-design-system-storybook canceled.
|
✅ Deploy Preview for spirit-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
exporters/variables-scss/src/formatters/__tests__/stylesFormatter.test.ts
Outdated
Show resolved
Hide resolved
exporters/variables-scss/src/generators/__tests__/fileGenerator.test.ts
Outdated
Show resolved
Hide resolved
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.
Please copy locally the exported files into the design-tokens package to see how it deals with the linters.
My points:
- @themes - please add a comment noting which theme is default. It can be basically anywhere in the file
- @global - please add empty line to the end of the file
- everywhere if the value is 0px, please remove the units
- gradients and shadows color hexes can be shortened (in globals) - can be solved in the last issue (if you don't solve it here, add it to other issue, please)
- in scss add comma after line-height in typography
- can forwards be sorted in global/index.scss
- in js eslint is failing on imports, probably not a problem of exporter, but I don't know what it is
- in js prettier expects spaces instead of tabs
- in js some objects can be simplified eg.
focusRing: focusRing,
->focusRing,
- I don't see all colors in theme-light-on-brand, i know not all colors are set, but even the set ones are not there
Thanks. I solved some things, some were outside the scope of this issue and I created a follow-up issue. See description above. |
exporters/variables-scss/src/generators/__tests__/stylesGenerator.test.ts
Outdated
Show resolved
Hide resolved
One little closing remark: I would prefer normalizing the Otherwise it is looking good. Great Job. Thanks 👍 |
Sorry, but I don't understand what you mean. Themes are loaded dynamically, including the name. So it depends on how it is named in the supernova. |
Ok, I see. So we need to standardize the naming of the themes or normalize them in JavaScript. Empty space can cause some troubles and I would like the directory structure to be normalized. I will create an issue/long topic for this. |
554d4fe
to
3e74fac
Compare
3e74fac
to
8a8f76e
Compare
8a8f76e
to
3e9a429
Compare
3e9a429
to
960986e
Compare
@crishpeen points:
Thing out of the scope will be solve in DS-1486
Description
Additional context
Issue reference