-
Notifications
You must be signed in to change notification settings - Fork 5
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
Lint CSS build with .browserslistrc #2128
base: main
Are you sure you want to change the base?
Conversation
|
package.json
Outdated
@@ -10,6 +10,7 @@ | |||
"test:components:all": "turbo test:components test:integration --concurrency 1", | |||
"format:all": "prettier --write .", | |||
"format:check:all": "prettier --check .", | |||
"stylelint": "stylelint \"packages/sit-onyx/dist/*.css\"", |
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 extend the turbo.json
with stylelint
depending on build
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.
@JoCa96 I am not sure if should mix up build and lint. What do you think about adding a separate styling step to the PR check pipeline?
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 meant that running stylelint
depends on build
, so this should be considered by turbo.
In the pipeline we can do it as part of the linting :)
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 added stylelint to check.yml
- should we still want to add it to turbo ?
- stylelint creates only warnings currently on standard output. That's also written to a text file. Do you have an idea what to do with the file ? 😃 Not sure about the possibilities here ..
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 would assume the command gives an appropriate exit code that we can use in the CI and local checks?
f3a5343
to
61b3d44
Compare
Relates to #1970
First do
pnpm i
, then runpnpm stylelint
to lint the Onyx CSS bundle based on the supported browsers in .browserslistrcThis does NOT lint the styles in single Vue components, since the plugin only supports CSS, no SCSS.
Currently only warnings are produced.
A sample output looks like this:
How does this help ?
We get almost 200 warnings, mainly because of Firefox ESR.
Ideas:
We could run stylelint in the pipeline, parse the output and only fail on problems in certain situations. For example when only the latest Firefox or Safari version is affected.
Or we can just keep the command in package.json and just run it manually to validate.
Checklist
apps/docs
)npx changeset add
if your changes should be released as npm package (because they affect the library usage)