Add Stylelint to gulp and CI #4567

alexgibson · 2017-01-04T18:00:08Z

Taking a look at adding Stylelint to our gulp/CI. The full list of rules that can be applied is here: http://stylelint.io/user-guide/rules/

Right now this is just linting our Sass files, but I'll take a look to see if it can do the Less files too. I'm trying to avoid purely stylistic things, but if there are things that we would normally pick out in code review anyway we should probably add them.

Like ESLint, you can make allowances for certain rules on a case by case basis using CSS comments: http://stylelint.io/user-guide/configuration/#turning-rules-off-from-within-your-css

We can also make certain rules only report as warnings, so not to break a build: http://stylelint.io/user-guide/configuration/#severities-error--warning

alexgibson · 2017-01-04T18:08:13Z

Would very much like input from @craigcook (and also @jpetto if he has any free mozorg time)

alexgibson · 2017-01-05T10:21:41Z

Ok I think I've made a decent run through of a basic rules list. Certain things (mostly stylistic) are only classed as warnings, but it's all up for discussion. I'd like to try and formalize some form of consensus on what we think is sensible, before moving on to actually fixing any linting errors or adding exceptions.

jpetto · 2017-01-05T16:05:32Z

.stylelintrc

+    "comment-no-empty": true,
+    "max-nesting-depth": [5, { "severity": "warning" }],
+    "no-duplicate-selectors": [true, { "severity": "warning" }],
+    "no-invalid-double-slash-comments": true,


Based on the lack of related errors when running locally, this appears to be smart enough to ignore double slash comments in .less and .scss files.

Yeah this only applies to CSS files, where it can cause issues. It ignores both Sass and Less comments, so maybe this is actually redundant here...

Are we linting post-processed .css files as well, or only the pre-processed source?

Are we linting post-processed .css files as well, or only the pre-processed source?

No, this would probably be a next step after this.

I'm actually going to leave this rule in, as we do have some regular .css files in /media.

jpetto · 2017-01-05T16:08:11Z

.stylelintrc

+    "keyframe-declaration-no-important": true,
+    "declaration-no-important": true,
+    "declaration-block-no-duplicate-properties": [true, { "severity": "warning" }],
+    "declaration-block-no-ignored-properties": true,


Wondering how this will play with workarounds for quirks in IE 9. Might illuminate some phony voodoo though. 😁

Yeah, will delve into the errors it's throwing currently - might need to drop this to a warning perhaps, and add exceptions for where it's required.

jpetto · 2017-01-05T16:09:08Z

.stylelintrc

+    "property-case": "lower",
+    "property-no-unknown": true,
+    "keyframe-declaration-no-important": true,
+    "declaration-no-important": true,


I really hope we can do away with !important, but I have a hunch we'll need it, at least for a while...

For now I plan to make exceptions for it in the download button styles, but let it throw an error everywhere else...

We might sometimes have to update the exceptions list (there are several !importants in the search stuff because Google) but hopefully that won't get out of hand.

Exceptions are added directly around the effected areas of CSS, so adding one for the Google search thing is pretty easy. I'd prefer to do this and try and keep the rule a strict "no" by default.

Ah, that makes sense. I was envisioning some kind of separate "ignore" list we'd have to maintain, which sounds like a bad idea. But just some special comment at the point of instance is much nicer. So in that case I also vote for keeping this as a full-blown error; !important should only be allowed under very special circumstances.

I've added some overrides to usages of !important where it's probably need it, so it's easier to see some practical examples.

jpetto · 2017-01-05T16:10:29Z

gulpfile.js

 gulp.task('static:clean', () => {
    return del(['static/**', '!static', '!static/.gitignore']);
 });

 gulp.task('default', () => {
    gulp.start('media:watch');
-    gulp.watch(lintPaths, ['js:lint']);
+    gulp.watch(lintPathsJS, ['js:lint']);


Should we add the CSS linter to the default task? Or is it too slow/cumbersome?

It currently takes around 10 seconds to lint everything, which is kinda a long time... could still add it though? Will see how it plays out 👍

I changed our default gulp task to lint both CSS and JS files, but for changed files only instead of the whole folders. This speeds things up considerably 👍

jpetto · 2017-01-06T15:46:37Z

This is looking really good to me. Do we just need to fix all current linter errors before it's ready to merge?

alexgibson · 2017-01-06T15:48:36Z

This is looking really good to me. Do we just need to fix all current linter errors before it's ready to merge?

I'm thinking I'll fix all the errors, and maybe a bunch of easy warnings. Then we could leave the rest up as "good-first-bugs"?

jpetto · 2017-01-06T15:57:34Z

I'm thinking I'll fix all the errors, and maybe a bunch of easy warnings. Then we could leave the rest up as "good-first-bugs"?

Sounds like a plan!

alexgibson · 2017-01-10T15:14:15Z

OK, i think i have this in a pretty good place. Once #4571 is merged I'll rebase and CircleCI should pass. Once this PR is merged, I'll file a tracking bug so contributors can help fix the linting warnings 👍

alexgibson · 2017-01-11T09:39:55Z

Ok this is passing in CircleCI 🎈

I think this is now ready for code review.

craigcook · 2017-01-19T14:35:37Z

media/css/base/menu-resp.less

@@ -186,11 +185,6 @@ html[dir="rtl"] #nav-main .submenu {
        padding: 0;
    }

-    .js #nav-main li .submenu {
-        xleft: 80px;
-        xtop: auto;


Wow, where did this even come from? Did I do that? Probably been here for years...

No idea - it was so mysterious I even searched in case there was some random proprietary thing I didn't know existed. Came up blank

craigcook

r+! 🚀

alexgibson added Do Not Merge ⚠️ WIP labels Jan 4, 2017

alexgibson force-pushed the stylelint branch from c599413 to dd93555 Compare January 5, 2017 10:19

alexgibson force-pushed the stylelint branch 4 times, most recently from 7f16b25 to 2f46e96 Compare January 5, 2017 15:47

jpetto reviewed Jan 5, 2017

View reviewed changes

alexgibson force-pushed the stylelint branch 7 times, most recently from 160664c to fd8671e Compare January 6, 2017 15:34

alexgibson force-pushed the stylelint branch 6 times, most recently from 9d8ec6a to 7f8ad03 Compare January 10, 2017 11:38

alexgibson force-pushed the stylelint branch 2 times, most recently from 6673853 to 33429aa Compare January 11, 2017 09:15

alexgibson removed Do Not Merge ⚠️ WIP labels Jan 11, 2017

alexgibson force-pushed the stylelint branch from 33429aa to 5c373f5 Compare January 18, 2017 14:27

Add Stylelint to gulp and CI

44e041d

alexgibson force-pushed the stylelint branch from 5c373f5 to 44e041d Compare January 19, 2017 14:28

craigcook reviewed Jan 19, 2017

View reviewed changes

craigcook approved these changes Jan 19, 2017

View reviewed changes

craigcook merged commit fbeae48 into mozilla:master Jan 19, 2017

alexgibson deleted the stylelint branch January 19, 2017 16:07

alexgibson mentioned this pull request Jan 19, 2017

open iframe on first run, sign in, animate #4579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Stylelint to gulp and CI #4567

Add Stylelint to gulp and CI #4567

alexgibson commented Jan 4, 2017 •

edited

alexgibson commented Jan 4, 2017

alexgibson commented Jan 5, 2017 •

edited

jpetto Jan 5, 2017

alexgibson Jan 5, 2017

craigcook Jan 5, 2017

alexgibson Jan 5, 2017

alexgibson Jan 6, 2017

jpetto Jan 5, 2017

alexgibson Jan 5, 2017

jpetto Jan 5, 2017

alexgibson Jan 5, 2017

craigcook Jan 5, 2017

alexgibson Jan 5, 2017

craigcook Jan 5, 2017

alexgibson Jan 6, 2017

jpetto Jan 5, 2017

alexgibson Jan 5, 2017 •

edited

alexgibson Jan 6, 2017

jpetto commented Jan 6, 2017

alexgibson commented Jan 6, 2017

jpetto commented Jan 6, 2017

alexgibson commented Jan 10, 2017 •

edited

alexgibson commented Jan 11, 2017

craigcook Jan 19, 2017

alexgibson Jan 19, 2017

craigcook left a comment

Add Stylelint to gulp and CI #4567

Add Stylelint to gulp and CI #4567

Conversation

alexgibson commented Jan 4, 2017 • edited

alexgibson commented Jan 4, 2017

alexgibson commented Jan 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson Jan 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpetto commented Jan 6, 2017

alexgibson commented Jan 6, 2017

jpetto commented Jan 6, 2017

alexgibson commented Jan 10, 2017 • edited

alexgibson commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigcook left a comment

Choose a reason for hiding this comment

alexgibson commented Jan 4, 2017 •

edited

alexgibson commented Jan 5, 2017 •

edited

alexgibson Jan 5, 2017 •

edited

alexgibson commented Jan 10, 2017 •

edited