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

Lint CSS build with .browserslistrc #2128

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

oemueller
Copy link
Contributor

@oemueller oemueller commented Nov 20, 2024

Relates to #1970

First do pnpm i, then run pnpm stylelint to lint the Onyx CSS bundle based on the supported browsers in .browserslistrc
This 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:

Unexpected browser feature "css-resize" is not supported by Safari on iOS
15.6-15.8,16.6-16.7,17.5,17.6-17.7,18.0,18.1

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.

@oemueller oemueller requested a review from a team as a code owner November 20, 2024 14:20
Copy link

changeset-bot bot commented Nov 20, 2024

⚠️ No Changeset found

Latest commit: 4f2f245

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@oemueller oemueller linked an issue Nov 20, 2024 that may be closed by this pull request
2 tasks
@@ -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\"",
Copy link
Collaborator

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

Copy link
Collaborator

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?

Copy link
Collaborator

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 :)

Copy link
Contributor Author

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 ..

Copy link
Collaborator

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?

Copy link
Contributor Author

@oemueller oemueller Dec 12, 2024

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 :-)

Bildschirmfoto 2024-12-12 um 09 52 38

@oemueller oemueller force-pushed the feat/1970-browser-compat-css-linting branch from f3a5343 to 61b3d44 Compare November 27, 2024 13:22
@oemueller oemueller force-pushed the feat/1970-browser-compat-css-linting branch from 61b3d44 to f3a32e5 Compare December 9, 2024 08:07
@oemueller oemueller force-pushed the feat/1970-browser-compat-css-linting branch from e200506 to 4f2f245 Compare December 12, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow up: Browser compatibility CSS linting
3 participants