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

Upgrade stylelint from v13 to v16 #741

Closed
wants to merge 7 commits into from
Closed

Conversation

chrispelzer
Copy link
Member

Fixes #634

Stylelint now includes a --fix option similar to phplinting so it is able to correct the errors without much issue

stylelint ./resources/scss/**/*.scss --fix

…a specific pattern for our classes in kebab-case by default, yet we use a variety of mixed at times depended on the needs of targeting external sources

fix: add "no-invalid-double-slash-comments" to allow `//` within our `scss` files due to the comments getting stripped out on processing
```
resources/scss/components/_flag.scss
  42:17  ✖  Unexpected unknown property "//"  property-no-unknown
```
```
resources/scss/components/_global-buttons.scss
  54:62  ✖  Unexpected unknown function "theme"  function-no-unknown
  72:62  ✖  Unexpected unknown function "theme"  function-no-unknown
```
```
resources/scss/components/_menu-top.scss
  28:12  ✖  Unexpected invalid media query "(max-width: theme('screens.mt'))"
```
@chrispelzer chrispelzer requested a review from a team as a code owner October 8, 2024 20:28
@chrispelzer chrispelzer closed this Oct 8, 2024
@chrispelzer chrispelzer deleted the feature/stylelint-v16 branch October 8, 2024 20:29
@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same
when pulling 88e6b54 on feature/stylelint-v16
into 57d8a81 on develop.

@nickdenardis
Copy link
Member

@chrispelzer
This looks good 👍

Can we add automatic fixing to the regular make command and a second 'dry' command to allow for a preview of changes like phplint?

Perhaps:

make stylelintdry
make stylelint

@nickdenardis
Copy link
Member

@chrispelzer
Upon further inspection, the automatic --fix doesn't seem to be working for me.

It looks like some of the CSS comments in this PR have moved from the // to the /* */ style, but not all of them.

After updating the .stylelintrc file to include
"no-invalid-double-slash-comments": true,

The errors are reported:

$ stylelint ./resources/scss/**/*.scss

resources/scss/components/_accordion.scss
  21:13  ✖  Unexpected double-slash CSS comment  no-invalid-double-slash-comments
  33:9   ✖  Unexpected double-slash CSS comment  no-invalid-double-slash-comments
  38:9   ✖  Unexpected double-slash CSS comment  no-invalid-double-slash-comments

...

✖ 46 problems (46 errors, 0 warnings)

Trying the automatic --fix produces the same warnings with no files changed:

$ stylelint ./resources/scss/**/*.scss --fix

resources/scss/components/_accordion.scss
  21:13  ✖  Unexpected double-slash CSS comment  no-invalid-double-slash-comments
  33:9   ✖  Unexpected double-slash CSS comment  no-invalid-double-slash-comments
  38:9   ✖  Unexpected double-slash CSS comment  no-invalid-double-slash-comments

...

✖ 46 problems (46 errors, 0 warnings)

Is there a specific way to denote which rules to automatically fix?

@chrispelzer
Copy link
Member Author

@nickdenardis

--fix only works on certain rules, others you need to correct yourself.

If you reverse one of the changes at the commit fix: various stylelint fixes based on stylelint ./resources/scss/**/*.scss --fix

and run it, you'll see it change.

$ stylelint ./resources/scss/**/*.scss

resources/scss/components/_formy.scss
  132:29  ✖  Expected quotes around "checkbox"  selector-attribute-quotes

✖ 1 problem (1 error, 0 warnings)
  1 error potentially fixable with the "--fix" option.

As for the CSS comments, it depends on if the comment is within a property {} or if it's within the outside.
It only matters for CSS, SCSS it doesn't, more than likely we should just switch all // over to /* */ since that is valid for CSS regardless of SCSS/CSS

@breakdancingcat
Copy link
Member

👍🏻

@nickdenardis
Copy link
Member

@chrispelzer
Got it. Let's get both of the commands in place to match the others:

make stylelintdry
make stylelint

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.

4 participants