-
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
|
@@ -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?
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 pushed a small update.
So the current state now is:
- stylelint is run in the pipleline as seperate check and returns errors if it finds unsupported CSS features
- right now there are errors because of some features we use like "css-selection"
- we can get rid if the errors by 1) adding the feature to the ignore array in stylelint.config.js or by changing the browserslist or by not using the feature :-)
we could also "mute" stylelint by turning errors into warnings but then why add it in the first place :-)
f3a5343
to
61b3d44
Compare
61b3d44
to
f3a32e5
Compare
also ignore safe css features
e200506
to
4f2f245
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.