feat: add stylelint for css-in-js #815
Conversation
| "indentation": null, | ||
| "selector-descendant-combinator-no-non-space": null, | ||
| "no-descending-specificity": null, | ||
| "no-duplicate-selectors": null, |
There was a problem hiding this comment.
I took all of these up until here from mc-fe
.stylelintrc
Outdated
| "selector-descendant-combinator-no-non-space": null, | ||
| "no-descending-specificity": null, | ||
| "no-duplicate-selectors": null, | ||
| "no-empty-source": null |
There was a problem hiding this comment.
I added this line because we are linting some files that ... don't have CSS. Like utils and such.
There was a problem hiding this comment.
The issue with excluding this is you would get an error from let's say, parseTime.js for having no css...
| moduleFileExtensions: ['js'], | ||
| modulePathIgnorePatterns: [ | ||
| 'dist', | ||
| '.spec.js', |
There was a problem hiding this comment.
I thought it made sense to exclude these
|
I think it would make sense to do the same setup on app-kit. @emmenko do you have any thoughts? |
.stylelintrc
Outdated
| "extends": [ | ||
| "stylelint-config-standard" | ||
| ], | ||
| "plugins": [ |
There was a problem hiding this comment.
| "plugins": [ | |
| "plugins": [ |
|
One caveat: this is only working with css made with string literals. // This is linted
<div css={css`
color: green;
`}>
Green
</div>
// this also is linted
const GreenText = styled.div`
color: green;
`;
// this is not linted
<div css={{
color: 'green',
}}>
Green
</div>
// this is not linted
<div style={{
color: 'green',
}}>
Green
</div>I'm trying to figure out a solution 😬 |
|
If only CSS could be typed... 😗😜 But anyway, shouldn’t VRTs provide feedback if the CSS is wrong? 🤔 |
|
Yeah, VRTs should give us an idea. Linting isn't just about finding errors though, it's also about finding duplicated css, etc. Stuff like this won't show up in VRTs but it's nice to get rid of it.
|
test/jest-styles-runner/run.js
Outdated
| @@ -0,0 +1,36 @@ | |||
| // copied from https://github.com/keplersj/jest-runner-stylelint/blob/master/src/run.js | |||
| // because it uses an older version of stylelint as a dep (not peer dep) | |||
| // we need new version of stylelint for css in js support | |||
There was a problem hiding this comment.
Why not using yarn resolutions?
There was a problem hiding this comment.
because I didn't know it existed 👍 🤣
| @@ -196,6 +203,9 @@ | |||
| "react-intl": "2.x", | |||
| "react-router-dom": "5.x" | |||
| }, | |||
| "resolutions": { | |||
| "jest-runner-stylelint/stylelint": "10.0.1" | |||
There was a problem hiding this comment.
gives this warning, but I think it's the intended result
warning Resolution field "[email protected]" is incompatible with requested version "stylelint@^8.3.1"
|
Alright, using the new version of stylelint solved the problems with trying to lint object styles |
Summary
When we migrated to @emotion, we removed stylelint. It makes sense now to add it back. This PR adds a script
yarn lint:css, and the necessary configs.It lints inline styles, styles with the css prop, and styled components.
Huzzah! 🧙
Current status of stylelint report
Right now there are some failing tests, so my idea is to first add the script, and then fix the stylelint errors in another PR.
It even found some invalid css!
There is an extra closing brace there. Cool!
Closes #815