Skip to content
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

Add Stylelint to gulp and CI #4567

Merged
merged 1 commit into from Jan 19, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions .stylelintrc
@@ -0,0 +1,43 @@
{
"rules": {
"color-no-invalid-hex": true,
"font-family-no-duplicate-names": true,
"font-family-name-quotes": ["always-where-recommended", { "severity": "warning" }],
"function-name-case": "lower",
"function-url-no-scheme-relative": true,
"function-url-quotes": ["always", { "severity": "warning" }],
"number-no-trailing-zeros": [true, { "severity": "warning" }],
"length-zero-no-unit": [true, { "severity": "warning" }],
"unit-case": "lower",
"unit-no-unknown": true,
"property-case": "lower",
"property-no-unknown": true,
"keyframe-declaration-no-important": true,
"declaration-no-important": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"declaration-block-no-ignored-properties": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"declaration-block-no-shorthand-property-overrides": true,
"declaration-block-single-line-max-declarations": [1, { "severity": "warning" }],
"declaration-block-trailing-semicolon": ["always", { "severity": "warning" }],
"block-no-empty": true,
"selector-no-empty": true,
"selector-pseudo-class-no-unknown": true,
"selector-pseudo-element-no-unknown": true,
"selector-pseudo-element-case": "lower",
"selector-type-case": "lower",
"selector-type-no-unknown": true,
"selector-max-empty-lines": 0,
"media-feature-name-case": "lower",
"media-feature-name-no-unknown": [true, {
ignoreMediaFeatureNames: ["min--moz-device-pixel-ratio"]
}],
"media-feature-no-missing-punctuation": true,
"stylelint-disable-reason": "always-before",
"comment-no-empty": true,
"max-nesting-depth": [5, { "severity": "warning" }],
"no-invalid-double-slash-comments": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"no-unknown-animations": true,
"no-extra-semicolons": true,
"no-missing-end-of-source-newline": [true, { "severity": "warning" }],
"no-eol-whitespace": [true, { "severity": "warning" }]
}
}
Expand Up @@ -29,7 +29,7 @@
<div class="container">
<div id="subscribe-wrapper">
{# L10n: Line break for visual formatting. #}
{{ email_newsletter_form(title=_('Keep up with<br> all things Firefox.'), button_class='button-light') }}
{{ email_newsletter_form(title=_('Keep up with<br> all things Firefox.'), button_class='button-light', spinner_color='#fff') }}
</div>
<div id="download-wrapper">
{{ download_firefox() }}
Expand Down
2 changes: 1 addition & 1 deletion bedrock/firefox/templates/firefox/new/scene1.html
Expand Up @@ -126,7 +126,7 @@ <h2 class="good">{{_('Freedom is yours') }}</h2>
</section>
<aside id="newsletter-subscribe">
<div class="container">
{{ email_newsletter_form(title=_('Keep up with<br> all things Firefox.'), button_class='button-light') }}
{{ email_newsletter_form(title=_('Keep up with<br> all things Firefox.'), button_class='button-light', spinner_color='#fff') }}
</div>
</aside>
</div>
Expand Down
1 change: 1 addition & 0 deletions circle.yml
Expand Up @@ -33,6 +33,7 @@ test:
pre:
- mkdir -p "$CIRCLE_TEST_REPORTS/django"
override:
- gulp css:lint
- gulp js:lint
- gulp js:test
- make test-image
41 changes: 38 additions & 3 deletions gulpfile.js
Expand Up @@ -9,14 +9,22 @@ const del = require('del');
const karma = require('karma');
const eslint = require('gulp-eslint');
const watch = require('gulp-watch');
const gulpStylelint = require('gulp-stylelint');

const lintPaths = [
const lintPathsJS = [
'media/js/**/*.js',
'!media/js/libs/*.js',
'tests/unit/spec/**/*.js',
'gulpfile.js'
];

const lintPathsCSS = [
'media/css/**/*.scss',
'media/css/**/*.less',
'media/css/**/*.css',
'!media/css/libs/*'
];

gulp.task('media:watch', () => {
return gulp.src('./media/**/*')
.pipe(watch('./media/**/*', {
Expand All @@ -33,17 +41,44 @@ gulp.task('js:test', done => {
});

gulp.task('js:lint', () => {
return gulp.src(lintPaths)
return gulp.src(lintPathsJS)
.pipe(eslint())
.pipe(eslint.format())
.pipe(eslint.failAfterError());
});


gulp.task('css:lint', () => {
return gulp.src(lintPathsCSS)
.pipe(gulpStylelint({
reporters: [{
formatter: 'string',
console: true
}]
}));
});

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).on('change', file => {
return gulp.src(file.path)
.pipe(eslint())
.pipe(eslint.format());
});

gulp.watch(lintPathsCSS).on('change', file => {
return gulp.src(file.path)
.pipe(gulpStylelint({
failAfterError: false,
reporters: [{
formatter: 'string',
console: true
}]
}));
});
});