-
Notifications
You must be signed in to change notification settings - Fork 53
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
chore: introduce CSS Modules #1265
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
f85eb76
to
a2b9711
Compare
a2b9711
to
90036d0
Compare
90036d0
to
d59618e
Compare
d9376ec
to
f966e1e
Compare
f966e1e
to
ee5b42f
Compare
ee5b42f
to
b574cd8
Compare
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.
Mostly everything looks good to me! I just had a few questions and suggestions, but nothing blocking. I'm looking forward to the simpler styling system. 🙂
], | ||
"**/packages/slice-machine/**/*.module.css": [ | ||
"prettier --write --ignore-unknown", | ||
"yarn workspace slice-machine-ui stylelint:precommit" |
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.
I think there was a discussion a while ago about removing pre-commit hooks considering we do these checks in CI. Is that still planned? If yes, I wonder if we should not add new pre-commit hooks.
(I prefer no pre-commit hooks since it slows the commit process down, but I don't have a full understanding of the history.)
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.
I think most team members, if not all, are okay to remove all our pre-commit hooks. However, I don't think it was listed somewhere so I've just created a Linear issue in our backlog: DT-1906. In the meantime, I think consistency has value and as TypeScript files are linted before committing, I think it's okay to do the same for CSS Modules.
packages/slice-machine/package.json
Outdated
"dev": "concurrently \"yarn typed-css-modules:watch\" \"next dev\"", | ||
"dev-cypress": "concurrently \"yarn typed-css-modules:watch\" \"next dev\" \"npm run dev --prefix ../../e2e-projects/cypress-next-app\"", |
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.
We can add names and colors to the concurrently
output using --names
and --prefix-colors
:
"dev": "concurrently \"yarn typed-css-modules:watch\" \"next dev\" --names \"typed-css-modules,next\" --prefix-colors auto",
"dev-cypress": "concurrently \"yarn typed-css-modules:watch\" \"next dev\" \"npm run dev --prefix ../../e2e-projects/cypress-next-app\" --names \"typed-css-modules,next,cypress-next-app\" --prefix-colors auto",
The names and colors make it easier to understand where a particular line comes from:
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.
I've made the change. Thank you!
reportInvalidScopeDisables: true, | ||
reportDescriptionlessDisables: true, | ||
rules: { | ||
"declaration-property-value-allowed-list": cssTheme, |
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.
Does this line ensure we follow the theme tokens? Like colors, spacing, sizing, etc.?
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.
Yes, exactly 🙂 It works fine, but I miss the TypeScript auto completion.
packages/slice-machine/package.json
Outdated
"stylelint": "15.11.0", | ||
"stylelint-config-css-modules": "4.3.0", | ||
"stylelint-config-prettier": "9.0.5", | ||
"stylelint-config-recommended": "13.0.0", |
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.
Is it possible to use the latest versions of these packages?
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.
Yes, I've made the change and stylelint-config-prettier
is no longer necessary as Stylelint has deprecated their formatting rules 🙂
packages/slice-machine/package.json
Outdated
"swr": "1.3.0", | ||
"theme-ui": "0.15.5", | ||
"typed-css-modules": "0.8.0", |
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.
Same as above: is it possible to use the latest version of this package?
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.
Yes, I've made the change.
b574cd8
to
dfa8cf1
Compare
dfa8cf1
to
f46ec04
Compare
Completes DT-1892.