-
-
Notifications
You must be signed in to change notification settings - Fork 176
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 for all the SCSS files #196
Conversation
src/app/components/home/layout.scss
Outdated
@@ -151,14 +151,14 @@ $sidebar-width: 190px; | |||
box-sizing: border-box; | |||
height: 100vh; | |||
max-width: $sidebar-width !important; | |||
// don't alphabetize for some reason |
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'll need to check this manually 🙆♂
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.
Resolved ✅
src/app/components/home/buttons.scss
Outdated
|
||
background: $gray-15; | ||
border: 1px solid $gray-30; // don't alphabetize this one |
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.
Check manually
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.
Resolved ✅
@@ -34,7 +34,9 @@ | |||
"electron:local": "npm run build:prod && electron .", | |||
"electron:linux": "npm run build:prod && electron-builder build --linux", | |||
"electron:windows": "npm run build:prod && electron-builder build --windows", | |||
"electron:mac": "npm run build:prod && electron-builder build --mac" | |||
"electron:mac": "npm run build:prod && electron-builder build --mac", | |||
"lint:scss": "stylelint \"./src/**/*.scss\"", |
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 include this in one of the building commands above? 🤔
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.
Would be good to automate the lint sometime, but I don't think it would be a good idea during the build.
I see the lint as a general house-cleaning procedure to keep our SCSS organized (more-readable / easier to search through). It's not important for the built app.
The biggest problem is that since the lint alphabetizes things, some properties might override the behavior of others above, resulting in an incorrect display of some elements. I'd rather have the build come out as-close to the dev environment as possible 😅
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.
To be clear, I meant lint:scss
just to display the warnings (I assume that's what it does), rather than the lint:scss:fix
version to actually change the code! 👍
That way you're more likely to notice, oh, I should change that, rather than remembering to run the linter! 😅
Stylelint is awesome for keeping all the CSS properties alphabetized for easier reference 👍
Now you can run
npm run lint:scss:fix
to fix all the SCSS 🚀There are some😅do not alphabetize
comments in the code I'll need to double-check and make sure nothing broke visuallyupdate: all works fine ✅