-
Notifications
You must be signed in to change notification settings - Fork 414
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
Stylelint preprocessor #140
Conversation
src/__tests__/css.spec.js
Outdated
'test.js': { | ||
template: ['\n color: blue;\n'], | ||
expressions: [], | ||
classname: header, |
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.
Shouldn't this be explicitly 'header__1r77qux'
?
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.
No, it should be the same classname as the one returned from css.named
.
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.
ok!
Can you also update the lockfile? |
Codecov Report
@@ Coverage Diff @@
## master #140 +/- ##
==========================================
+ Coverage 97.53% 97.58% +0.04%
==========================================
Files 19 19
Lines 406 414 +8
Branches 83 87 +4
==========================================
+ Hits 396 404 +8
Misses 10 10
Continue to review full report at Codecov.
|
stylelint-config.js
Outdated
'at-rule-semicolon-newline-after': 'always', | ||
'max-empty-lines': 1, | ||
'no-empty-source': null, | ||
'length-zero-no-unit': true, |
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.
isn't this a style thing? 😬
src/css.js
Outdated
@@ -5,6 +5,12 @@ import slugify from './slugify'; | |||
import sheet from './sheet'; | |||
import { getFramesFromStack, enhanceFrames } from './babel/lib/errorUtils'; | |||
|
|||
const rawStyles = {}; |
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.
since we usually do the caching stuff inside sheet
, I'd suggest to move this there
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.
No, styles in sheet are already processed.
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.
I mean to add a new variable for unprocessed styles in sheet instead of in css
src/tools/stylelint-preprocessor.js
Outdated
} | ||
|
||
export default function linariaStylelintPreprocessor(/* options */) { | ||
process.env.LINARIA_OVERWRITE_BABEL_PRESET = JSON.stringify({ |
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.
maybe call this LINARIA_BABEL_PRESET_OPTIONS
or LINARIA_BABEL_PRESET_OVERRIDES
stylelint-config.js
Outdated
'at-rule-semicolon-newline-after': 'always', | ||
'max-empty-lines': 1, | ||
'no-empty-source': null, | ||
'length-zero-no-unit': true, |
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.
let's remove style rules such as max-empty-lines
, at-rule-semicolon-newline-after
, length-zero-no-unit
and provide a base config which caches only errors. style rules will be addressed by prettier anyway.
website/.stylelintrc
Outdated
@@ -0,0 +1,7 @@ | |||
{ | |||
"processors": ["linaria/stylelint-preprocessor"], |
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.
can processors
and syntax
be inside our config? does it make sense?
- run: | | ||
cd website && yarn run build:all && cd .. | ||
- run: cd website && yarn run build:all && cd .. | ||
- run: cd website && yarn run lint-css && cd .. |
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.
maybe we should do
- run : |
cd website
yarn run build:all
yarn run lint-css
cd ..
instead?
.eslintignore
Outdated
@@ -4,3 +4,4 @@ coverage/ | |||
flow-typed/ | |||
node_modules/ | |||
vendor/ | |||
website/ |
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.
we shouldn't ignore website from linting
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.
we should otherwise it will throw some weird errors
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.
✖ 551 problems (494 errors, 57 warnings)
all of them from website
Closes #8
We need to figure out some nice stylelint config.