-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Require stylelint v14 for stylelint config and scripts #38091
Conversation
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
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.
Thanks @ocean90, looks good to me, 1 comment, not a blocker though, approving and can update the PR if needed, otherwise merge away 💯
// stylelint-disable no-invalid-position-at-import-rule | ||
@import "../some/url"; | ||
@import "../another/url"; | ||
// stylelint-enable no-invalid-position-at-import-rule |
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.
With the addition of packages/stylelint-config/test
in .stylelintignore
in his PR at https://github.com/WordPress/gutenberg/pull/38091/files#diff-36ae73c99ebd1b4072a3164e014df72eb094703df4548dd51938aa7faaa7f8d2R1 are the ignore directives needed 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.
Yes, the .stylelintignore
is only used when running npm run lint-css
in the project root but not when running npm run test-unit packages/stylelint-config
for the tests.
The rule is new in v5, see stylelint-scss/stylelint-config-recommended-scss@1e40cc1. Instead of removing the lines I added the comments for now.
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.
Cool, makes sense
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.
Nice, thank you for bringing it up to date 🙇♂️
@ocean90, it looks like the recently merged Jest major version upgrade introduced merge conflict. The good news is that we should be up to date with most of the tools used in |
058744c
to
075ec59
Compare
I have rebased the branch. 🤞 |
Description
This is a follow-up for #36518. As mentioned by @WraithKenny in #36514 (comment) the current dependencies are not compatible with v14. Therefore we should increase the minimum peer dependency of
stylelint
to14.2.0
and thus update scripts to use v14 too.While working on this I noticed that the current stylelint repo in this config was not using the SCSS preset. This seems to be the reason why suddenly a few errors came up since I had to switch to the SCSS preset otherwise stylelint is no longer able to parse SCSS files, see official migration guide to v14 for details.
To update dependencies the I have followed the steps mentioned in https://github.com/WordPress/gutenberg/blob/HEAD/packages/README.md.
How has this been tested?
Manually by running
npm run lint-css
in the project.Screenshots
Types of changes
Breaking change
Checklist:
*.native.js
files for terms that need renaming or removal).