-
Notifications
You must be signed in to change notification settings - Fork 844
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
Replace sass-lint
dependency with stylelint
#6470
Conversation
- see https://github.com/stylelint/stylelint/blob/main/docs/user-guide/get-started.md#using-a-community-shared-config - note that adding postcss@8 is necessary for the scss config to work - see stylelint-scss/stylelint-config-standard-scss#5
- JS file helps us do things like leave meaningful comments, which JSON doesn't allow
- previously ignored files were added due to vscode's overeager linting - it looks like said plugin no longer does so, so we can remove non `.scss` files - port over sass-lint excludes - add missed datepicker dir - add non-src files - remove no-longer-existant chart dir
+ fix issues caught by stylelint but not by sass-lint - unlike sass-lint, stylelint checks indentation of comments and also checks for tabs vs spaces + fix a few unnecessary `()`s, and add a disable because stylelint is confused by scss maps + remove unnecessary indentation disables
+ add skip for Sass scenarios where `"` has to be used for interpolation
- there isn't a handy `1tbs` option like eslint/sass-lint, so we need to combine a few different rules to get the effecrt we want + fix various instances where stylelint caught unnecessary double spaces before braces
+ include some rules that stylelint is more strict about than sass-lint was
- sass-lint was incorrectly marking pseudo *classes* as needing two `::`, whereas only pseudo *elements* need two `::`s. stylelint appears to correctly catch this OOTB @see https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Selectors/Pseudo-classes_and_pseudo-elements
…espace - prettier does the same/similar for JS/CSS-in-JS in any case
- already appears to be something we do or was enforced by sass-lint, but stylelint needs the explicit rule
- specifically around empty comments & spacing around comments - I find them to be valid and inline with JS linting
- without these added rules, we get stylelint errors for how we currently write our CSS - although we may want to consider removing these in the future & using more modern syntax
- that were not previous enforced by sass-lint - we can consider turning them on at a later time if we want
+ remove unnecessary disables
- default with sass-lint but not with stylelint or existing configs + remove unnecessary disables + add newly necessary disables (some it's unclear why sass-lint didn't catch, and some are due to sass syntax, which stylelint unfortunately does not care about)
- by default, stylelint considers comments to not make the block empty
+ remove disables
- this is not perfect - stylelint does not appear to have a 1:1 with sass-lint on this, but it's somewhat close and will become less of an issue as we migrate to Emotion + remove disable lines that no longer apply
- no-mispelled-properties is simply wrong / out of date - no-warn was already disabled in -sass-lint.yml - empty-args isn't useful in a post-sass world
- there's no corresponding stylelint rule, and it feels pedantic / not terribly useful
- there's no corresponding stylelint rule, and it feels pedantic / not useful
- these don't appear to be enforced by stylelint, and as such nothing needs to be done
- It's not clear to me why that plugin was added or what it does, and it was last updated 5 years ago and uses a super old version of stylelint (8.x), so I'm removing it
Preview documentation changes for this PR: https://eui.elastic.co/pr_6470/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6470/ |
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.
Tested locally using VS Code and LGTM.
Are there any CSS lint rules that we currently linting that we do not want to be linting?
I just found one rule that I would remove it.
- thanks Elizabet!
Preview documentation changes for this PR: https://eui.elastic.co/pr_6470/ |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6470/ |
Summary
This PR is part of a higher-level goal of removing our
node-sass
dependency (see #6442).sass-lint
has been deprecated for over 5 years now and its removal is long overdue.stylelint
is generally held to be the updated replacement, and supports CSS-in-JS as well as Sass.📚 Helpful stylelint docs/reading:
What this PR does:
Dependencies:
sass-lint
andsass-lint-auto-fix
(stylelint
has a OOTB--fix
flag)prettier-stylelint
(it's not clear to me why this package was added or what it does, and it was last updated 5 years ago and uses a super old version of stylelint (8.x), so I opted to remove it)Rules:
sass-lint
rules tostylelint
equivalents (or as close as I could get it)sass-lint
default rules (that I know of and saw) tostylelint
rule equivalents, ifstylelint
did not have said rules enabled by defaultstylelint
default rules, ifsass-lint
was not previously configured to enforce themWhat this PR does not do:
stylelint
equivalent (that I also deemed not terribly useful) These rules are:What I'm looking for feedback on:
QA
yarn compile-scss
and compare output diff of built EUI.css
file tomain
to ensure no changes::
pseudo element fixes,+
spacing, duplicate selector)General checklist
- [ ] A changelog entry exists and is marked appropriatelyDev-only changes