Grunt: Add a stylelint precommit task to check for CSS syntax errors

[helen]

We need to do a better job of catching sad syntax errors and problems in our CSS before commit - things like parse errors, empty rules, units on zero values, and possibly duplicate properties (when alone, not as a part of a group). There may also be a thing or two that we could enforce per our own standards, such as requiring a comment to follow any declaration with !important.

CSSLint seems to do most of these, provided we turn off the majority of its checks. Many of those checks (vendor prefixes, selector specificity, etc.) either are not relevant to our set up (due to Autoprefixer, for example) or are just not feasible given our current CSS and possibly not desirable for this project.

Interested to know if there are any other tools out there that perhaps fit the job better, and defining the parameters of what we would like to check.

[x2048]

I tried adding csslint to grunt and ran into the following problems:

​http://stackoverflow.com/questions/15626062/getting-errors-in-the-css-linter-when-using-media-queries
​https://github.com/CSSLint/csslint/issues/295

These problems occur even with all linting settings set to false.

To use CSSLint:
1) all media queries will need to be rewritten to not include spaces between parentheses, or CSSLint will show errors.
2) the @ms-keyframes styles will need to be moved to a separate file (that doesn't get linted), or added to the auto-prefixer.

[x2048]

It appears that @-ms-keyframes is not actually used in any browser, and can safely be deleted:
​http://css-tricks.com/snippets/css/keyframe-animation-syntax/

I put together a really small library that parses CSS and reports errors according to the WP guidelines.
Currently it handles "camelCaseSelectors", "selectors_with_underscores", and ".multiple, .selectors, .on-one-line".
Would this be useful, and should I add more things(like "!important requires comments")?

There's a patch that installs these two grunt tasks.
Try them out with "grunt csslint" and "grunt wp-css-check".

(btw, I'm new to WordPress core development - I only started this week. Please tell me if I'm doing something wrong)

[x2048]

Adds CSSLint and a small custom WP CSS style checker as grunt tasks. Removes space around parens in media queries.

[GaryJ]

​grunt-wp-css is a wrapper for CSSComb (and ​currently CSSBeautify until an issue in csscomb.js is fixed) and includes a couple of configs (same, other than default vs alphabetical sort order).

Whether it's csslint or csscomb, I'd like to see a standalone grunt package ( like grunt-wp-css) which then core and plugins and themes could use, with minimal configuration, rather than have to set up csslint / csscomb grunt packages with duplicitous configs.

[helen]

Replying to GaryJ:

Specifically not tackling standards here, rather syntax checking, but good to know for future reference.

[helen]

I realized that the mention of our standards in the ticket description is probably a little misleading, but in essence I'd like to start with error checking here and then move on to tasks that would actually make changes separately.

[x2048]

@helen What steps do I need to take in order to have ​https://github.com/x1024/wp-css-check be considered as a solution for the task?

[GaryJ]

CSSComb does have a -l or --lint option, but it's not very helpful at the moment, not reporting lines and columns etc. CSSLint would be favourable over CSSComb just for error checking of specific non-standards issues.

[helen]

Replying to x2048:

@helen What steps do I need to take in order to have ​https://github.com/x1024/wp-css-check be considered as a solution for the task?

Mention it here, as you've done :)

[x2048]

Nice :)

So, I started building this after looking over the WP issues at "Contributor Day" at WordCamp 2014.
The submitted patch installs a grunt task that calls wp-css-check.

I'd be happy to continue developing it, if there's interest.

[jorbin]

Thanks for the start on this x2048!

I think error checking is something we can do just by using something like ​https://github.com/reworkcss/css to see if the AST is built properly and if it errors, we know that we have an error. This seems to be the base of what you are using x2048.

Having a style checker seems like a separate issue and thus should be a separate ticket.

[nacin]

Replying to x2048:

1) all media queries will need to be rewritten to not include spaces between parentheses, or CSSLint will show errors.

No thanks. :) I patched CSSLint for this: ​https://github.com/CSSLint/parser-lib/pull/125

I think CSSLint is actually feasible here. Here's what I'm using, with good effect:

{
	// I used jQuery UI as a starting point
	"adjoining-classes": false,
	"box-sizing": false,
	"compatible-vendor-prefixes": false,
	"duplicate-background-images": false,
	"import": false,
	"important": false,
	"outline-none": false,
	"overqualified-elements": false,
	"text-indent": false,
	// Additional
	"ids": false,
	"box-model": false,
	"qualified-headings": false,
	"unique-headings": false,
	"universal-selector": false,
	"regex-selectors": false,
	"floats": false,
	"font-sizes": false
}

With my patch applied to parserlib, this results in:

• errors for unknown moz-document and ms-keyframes
• no other errors
• 65 warnings

The warnings all seem like good things to fix:

• non-adjacent duplicate properties (5)
• unknown property values (3)
• no fallback colors for RGBA usage (18)
• one vendor-prefixed property without the standard property
• one verbose property usage when shorthand will do (1)
• zero units (17)
• some display property grouping warnings (19)
• one unqualified attribute (this one may be a bug)

[jorbin]

Replying to nacin:

With my patch applied to parserlib, this results in:

• errors for unknown moz-document and ms-keyframes
• no other errors
• 65 warnings

The warnings all seem like good things to fix:

• non-adjacent duplicate properties (5)
• unknown property values (3)
• no fallback colors for RGBA usage (18)
• one vendor-prefixed property without the standard property
• one verbose property usage when shorthand will do (1)
• zero units (17)
• some display property grouping warnings (19)
• one unqualified attribute (this one may be a bug)

That seems less like syntax checking to me and more like stylistic checking. Much like we use jsvalidate and jshint separately, I think this is two different separate tasks.

[nacin]

Replying to jorbin:

That seems less like syntax checking to me and more like stylistic checking. Much like we use jsvalidate and jshint separately, I think this is two different separate tasks.

Hence the difference between ERROR and WARNING in CSSLint.

[Michael Arestad]

CSSlint is pretty handy for error-checking. I use it in Atom with several of the checks turned off. It's a good thing to include.

What would really be handy: A tool that tracked the number of each type of selector, the speed of each page, and potentially framerate, and the slowest selectors. I will buy whoever builds/implements this a nice bottle of your favorite type of alcohol.

[netweb]

Replying to nacin:

Replying to x2048:

1) all media queries will need to be rewritten to not include spaces between parentheses, or CSSLint will show errors.

No thanks. :) I patched CSSLint for this: ​https://github.com/CSSLint/parser-lib/pull/125

Media Queries are currently defined in WP Core CSS coding standards as to not include spaces between parentheses:
Source: ​https://make.wordpress.org/core/handbook/coding-standards/css/#media-queries

@media all and (max-width: 699px) and (min-width: 520px) {
 
        /* Your selectors */
}

Note: This WP Core CSS Standard will need updating once safe to do so, currently updates/changes to the CSS Handbook are not allowed, see ​Core Contributor Handbook spreadsheet. CC: Ping @samuelsidler

[netweb]

In 29792.patch:

• Adds .csslintrc file with the rules per Nacin's suggested settings in comment 11, also explicitly declares the other available ​rules as true for completeness/verbosity

• Adds grunt-contrib-csslint to package.json ​https://github.com/gruntjs/grunt-contrib-csslint

• New Grunt task grunt csslint

• Grunt sub tasks grunt csslint:core and grunt csslint:build to lint CSS files in respective /src and /build folders
  • wp-admin/css/*.css
  • wp-content/themes/twenty{ten,eleven,twelve,thirteen,fourteen,fifteen}/**/*.css
  • wp-includes/css/*.css

• Adds grunt csslint:core to precommit task grunt precommit

• Adds grunt csslint:build to both grunt rtl and grunt colors tasks as a psuedo CSS validator after CSSJanus, SASS, and AutoPrefixer compilers have parsed existing CSS/SASS files.

• Adds grunt csslint:core to Grunt build task grunt/grunt build before other CSS tasks are run

• Adds grunt csslint:build to Grunt build task grunt/grunt build after other CSS tasks have ran

---

Notes:

• If testing this patch in your workflow you may need to add --force to the task grunt precommit to get past any current errors until Core patches existing CSS issues per the CSS Lint rules here

• A few tweaks are needed to existing Grunt watch tasks to include support for this, including it in this 29792.patch kind of made things rather funky as most of the existing CSS tasks need updating also to accommodate this.

• Included in the patch are all WordPress Core themes, Twenty Ten, Eleven, Twelve, Thirteen, Fourteen, and Fifteen

[x2048]

A new version of the wp-css-check tool. Now also checks for CSS property syntax (e.g. mistakes like "width :100px"). Also checks for !imporant properties without comments.

[iseulde]

has-patch 4.2-early so moving to 4.2.

[F J Kaiser]

Some notes I got for this ticket as my workflow involves all those tools.

• Use one Grunt plugin for one task. Linters are good for linting, cleaners for cleaning, etc.
• CSSComb should be used to define the order or CSS definitions per class, id, etc.
• Linters are best used after each task has finished. Tasks can go wrong - seriously wrong. And Linters are there for the rescue. A stepped process with Linters running after each step helps debugging such problems in seconds. Without that: Happy debugging, there goes your beer with friends.

Summed up, the "no headaches" workflow is:

Clean CSS files in dev folder
Lint original files (SCSS/SASS/LESS Linter)
Preprocess (SCSS/SASS/LESS task) to current folder
Lint resulting CSS > Run CSSComb in current folder
Lint CSS
Run Minification to .cache folder
Lint CSS
Concatenate all resulting files to dist folder
Optionally remove unused CSS definitions

Tools like these are best served as git hooks. I know SVN, etc., but for those who are interested, here's ​our Git PHP Hooks tool, the ​accompanying library and ​our Grunt GitHooks task.

[nacin]

Quick update on ​https://github.com/CSSLint/parser-lib/pull/125. They want some changes to it, but it requires re-doing how a bunch of the tests currently work (not just the ones I added), so I think it'll get merged. Waiting to hear back. If anyone wishes to play with the tests over there, though, feel free.

[lkraav]

Replying to GaryJ:

CSSComb does have a -l or --lint option, but it's not very helpful at the moment, not reporting lines and columns etc. CSSLint would be favourable over CSSComb just for error checking of specific non-standards issues.

Yes, this is ​https://github.com/csscomb/csscomb.js/issues/245

I did extensive research on csslint vs csscomb yesterday for my own company. I found csscomb to be significantly further along for being able to define an exact detailed coding standard configuration and then having the tool not only lint to it, but automatically beautify your source and it actually coming out nearly 100% right. Yandex guys have built csscomb on ​gonzales-pe, an AST generator which looks like a building block sitting at the same level where ​parser-lib is sitting for csslint.

Both are very good tools and might very likely coexist in the build chain, but both also have a number of outstanding bugs that could really use attention. If there was a crowdfund for getting work done on them, I'd contribute in an instant. csscomb is the current choice for my team setup here, but as I'm understanding both a lot better now, I will be contributing to both and re-evaluating periodically. Wasn't able to find anything else at the same maturity level.

[lkraav]

​https://github.com/postcss/postcss may also be of interest here.

[netweb]

Replying to lkraav:

... both also have a number of outstanding bugs that could really use attention.

This is becoming an important issue for us, our tools need to be both functional and maintained and is one part of the reasoning for the change in #31332

Replying to lkraav:

​https://github.com/postcss/postcss may also be of interest here.

The proposed RTLCSS change in #31332 also uses POSTCSS also :)

[nacin]

Replying to nacin:

Quick update on ​https://github.com/CSSLint/parser-lib/pull/125.

This was merged.

[netweb]

Replying to nacin:

Replying to nacin:

Quick update on ​https://github.com/CSSLint/parser-lib/pull/125.

This was merged.

It was, sadly a new version, v0.2.6 that includes these changes is still yet to be released:
​https://github.com/CSSLint/parser-lib/issues/135 ​https://github.com/CSSLint/parser-lib/releases

Peering into the rabbit hole, we then need v0.2.6 merged into a new release of CSSLint before grunt-contrib-csslint can be updated to include these changes...

The latest release of CSSLint is v0.10.0 released ~18 months ago, I couldn't find anything readily available as a roadmap of determining when the next release is scheduled though.

Edit: CSSLint v0.11.0 outstanding issues: ​https://github.com/CSSLint/csslint/milestones/0.11.0 of which many have not been updated in quite a few months, I think we may need to seek an alternative to CSSLint, just as I noted in comment:25 above we are need to use projects that are routinely maintained and updated.

[helen]

Punting - @netweb (or anyone else), if you have a chance to look for more maintained CSS linters, that would be wonderful.

[netweb]

Current situation still stands, looking for a more maintained CSS linter

[netweb]

The following which is based on PostCSS (We like PostCSS tools) looks very promising as a replacement:

​https://github.com/stylelint/stylelint ​https://stylelint.github.io/

Notes: It has not yet been released, actively being worked on as a replacement for CSSLint and CSSComb

[netweb]

Using ​https://github.com/stylelint/stylelint with ​https://github.com/stylelint/stylelint-config-wordpress

Modern CSS linter.
A PostCSS plugin
Drops in seamlessly alongside our existing Autoprefixer PostCSS plugin

Available rules ​https://github.com/stylelint/stylelint/blob/master/docs/rules.md
Currently defined WordPress CSS rules ​https://github.com/stylelint/stylelint-config-wordpress/blob/master/index.js

In 29792.1.diff

• 29792.1.diff is ideally what we would commit to WordPress once our rules have been finalized
• Uses the rules per the currently defined rules in ​stylelint-config-wordpress
• ~5,000 line output of grunt postcss ( core and colors ) ​https://gist.github.com/ntwb/fe5383e53092ea41aec4

In 29792.2.diff an extra two rules are disabled:

• 29792.2.diff gives us a way to test and hack on our rule definitions without needing to wait or change upstream rules in stylelint-config-wordpress
• 240 instances of rule-nested-empty-line-before (​definition)
• 647 instances of rule-non-nested-empty-line-before (​difinition)
• ~4,000 line output of grunt postcss ( core and colors ) ​https://gist.github.com/ntwb/6f1a7e2297ce3c6c2c7d

To test either of these patches, first run npm install then running grunt precommit, grunt postcss:core, grunt postcss:colors, or grunt postcss (combined core and colors) should be enough to get you overwhelmed in CSS rule errors ;)

The rules in ​https://github.com/stylelint/stylelint-config-wordpress were created based on our CSS coding standards
​https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/.

Now to test, tweak, and iterate these stylelint rules, our handbook CSS coding standards, and of course our CSS :)

[netweb]

I've just refreshed 29792.1.diff and 29792.2.diff now that v0.1.0 of the config is released and published on npmjs.org

​https://www.npmjs.com/package/stylelint-config-wordpress

[netweb]

Related: #26905 Follow CSS coding standards for SASS/SCSS files

[netweb]

I’ll refresh the patch here in a day or two once the direction on a couple of bug fixes upstream are known.

For the moment some quick stats compared my previous comments above where we had 4,000/5,000 warnings this is now down to ~200 warnings 29792-warnings-without-vendor-prefix.txt or 1,000 warnings (29792-warnings-with-vendor-prefix.txt) when including vendor prefixes (waiting on upstream bug fix for vendor prefixes).

A preliminary CSS patch 29792-css-patch.diff is also attached that shows what we’ll be looking to commit in core. I’m attaching this CSS file now for review, if there are changes in this that you don’t think should be there and we should have more rules to allow for that particular CSS formatting let me know and we can create more rules or iterate on the existing rules to allow these changes.

Bonus Points: The above also now includes SCSS linting and fixes that will be part of #26905

@Helen and @Nacin, if you'd be so kind to take a peek inside 29792-css-patch.diff and let me know any pain points you can foresee I can then continue on iterating these rules :)

[nacin]

This looks great, @netweb!

Rule questions:

• src/wp-admin/css/colors/_admin.scss:83 uses // as a commenting style. I know this is allowed in SCSS, but do these rules mandate // over /* */? Should we always use /* */, since it's ultimately CSS? I don't see what stylelint rule this corresponds to.

• I noticed the removal of spaces within parentheses. Spacing within parentheses is a thing WP likes to do. Can we keep them? I wonder if enforcing them ends up being less changes. (This would mean function-parentheses-space-inside: always.)

• The requirement for a leading 0 for a decimal value (.1s or .25) seems weird. They get minified out anyway. And, it seems, we omit them all over the place. Which is more common in our codebase? What's common in the real world? (This would mean function-parentheses-space-inside: always).

• string-quotes -- What is more common in our CSS, single quotes or double quotes? We already prefer single quotes in JS (unlike the jQuery project) and in PHP, so should we just keep using it in CSS too? Is there a reason to not be consistent? (I think double quotes look more natural when surrounding a font name, I'm just asking.)

Where we should change our CSS to conform to the rules, rather than add ignore rules:

• We should only ignore declaration-block-semicolon-newline-after in L10n, and even then, I don't care. (It's actually done on purpose to group declarations together, so it's fine.)

• For @-webkit-keyframes dice-color-change, we don't align things the way we do for @-webkit-keyframes customize-reload. We might as well make them standardized so they ignore the same things.

• We shouldn't ignore number-zero-length-no-unit. Remove the units.

[GaryJ]

Replying to nacin:

• src/wp-admin/css/colors/_admin.scss:83 uses // as a commenting style. I know this is allowed in SCSS, but do these rules mandate // over /* */? Should we always use /* */, since it's ultimately CSS? I don't see what stylelint rule this corresponds to.

// gets removed by default during the SCSS->CSS compiling, so it never makes it into CSS. There's a functional difference at play here, not just a code style preference.

• The requirement for a leading 0 for a decimal value (.1s or .25) seems weird.

That's part of the documented ​WP coding standards for CSS: "Use a leading zero for decimal values, including in rgba()."

• string-quotes -- What is more common in our CSS, single quotes or double quotes? We already prefer single quotes in JS (unlike the jQuery project) and in PHP, so should we just keep using it in CSS too? Is there a reason to not be consistent? (I think double quotes look more natural when surrounding a font name, I'm just asking.)

Already documented in the CSS standards as: "Use double quotes rather than single quotes, and only when needed, such as when a font name has a space.". Custom fonts, using a single name, don't technically need any quotes, but some folks favour them for some reason.

[helen]

Squee. :)

Replying to nacin:

• src/wp-admin/css/colors/_admin.scss:83 uses // as a commenting style. I know this is allowed in SCSS, but do these rules mandate // over /* */? Should we always use /* */, since it's ultimately CSS? I don't see what stylelint rule this corresponds to.

Per @GaryJ, they are functionally different, but I'm not sure if/when we would have rules in our very limited use of SCSS that shouldn't be in the unminified compiled CSS.

• I noticed the removal of spaces within parentheses. Spacing within parentheses is a thing WP likes to do. Can we keep them? I wonder if enforcing them ends up being less changes. (This would mean function-parentheses-space-inside: always.)

Let's please keep them and allow minification to strip them for minified files. As with PHP and JS, it's great for readability, which is extremely important for development files.

• The requirement for a leading 0 for a decimal value (.1s or .25) seems weird. They get minified out anyway. And, it seems, we omit them all over the place. Which is more common in our codebase? What's common in the real world?

My recollection of why we went this route in our coding standards was also for human readability: . is easy to miss, 0. less so. I'd prefer to add them in where missing.

• string-quotes -- What is more common in our CSS, single quotes or double quotes? We already prefer single quotes in JS (unlike the jQuery project) and in PHP, so should we just keep using it in CSS too? Is there a reason to not be consistent? (I think double quotes look more natural when surrounding a font name, I'm just asking.)

Double quotes somehow "feel" more right in CSS, I think possibly because it's a standard in many other projects. It's not just voodoo personal preference, promise :)

• We shouldn't ignore number-zero-length-no-unit. Remove the units.

Patch seems to remove them - did I miss something?

[netweb]

@Helen

Could you please update both the description and example for "multiple comma-separated values" in the CSS handbook ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#values

Description: (Bold emphasis is mine and I'm seeking to clear up any ambiguity here)

Multiple comma-separated values for one property should be separated by either a space or a newline, including within rgba(). Newlines should be used for lengthier multi-part values such as those for shorthand properties like box-shadow and text-shadow. Each subsequent value after the first should then be on a new line, indented to the same level as the selector and then spaced over to left-align with the previous value.

Example: (I've copied and pasted verbatim the example source, white space for the last two lines should be updated)

.class { /* Correct usage of quotes */
        background-image: url(images/bg.png);
        font-family: "Helvetica Neue", sans-serif;
}

.class { /* Correct usage of zero values */
        font-family: Georgia, serif;
        text-shadow: 0 -1px 0 rgba(0, 0, 0, 0.5),
                                           0 1px 0 #fff;
}

Updated Example 1: (This example is based on the current description as it stands)
( 2 tabs and 5 spaces for that 2nd last line, but whitespace is weird here on Trac)

.class { /* Correct usage of quotes */
        background-image: url(images/bg.png);
        font-family: "Helvetica Neue", sans-serif;
}

.class { /* Correct usage of zero values */
        font-family: Georgia, serif;
        text-shadow: 0 -1px 0 rgba(0, 0, 0, 0.5),
                     0 1px 0 #fff;
}

Your commit message in r34637:

Normally when there are multiple comma-separated values in CSS, each one would go on its own indented line. However, Autoprefixer appears to be tripping up on gradients at the moment, so it's going to stay on one line until we investigate upstream.

As such, what I actually expect the example to be is, and is also what the majority of WordPress' CSS code base uses:
( 2 tabs indentation for the 2nd and 3rd last lines)

.class { /* Correct usage of quotes */
        background-image: url(images/bg.png);
        font-family: "Helvetica Neue", sans-serif;
}

.class { /* Correct usage of zero values */
        font-family: Georgia, serif;
        text-shadow: 
                0 -1px 0 rgba(0, 0, 0, 0.5),
                0 1px 0 #fff;
}

And finally the CSS handbook docs I believe should also match this and be something like this: (Emphasis mine)

Multiple comma-separated values for one property should be separated by either a space or a newline, including within rgba(). Newlines should be used for lengthier multi-part values such as those for shorthand properties like box-shadow and text-shadow. Each value should be on a new line, indented to the next tab indentation level of the parent selector and then left-aligned for each line.

If the above is not the case then we'll need to update both the Autoprefixer issue (​527) and pull request (​528) opened upstream by @Ocean90 to match our CSS standards.

Edit: See also the Stylelint ​declaration-colon-newline-after and ​declaration-colon-space-after rules.

Edit: Support for this was added to stylelint-config-wordpress in ​stylelint/stylelint-config-wordpress/pull/12

[netweb]

Replying to slackbot:

This ticket was mentioned in ​Slack in #core by jorbin. ​View the logs.

Following up on questions raised in the above chat log:

jorbin [6:33 AM] For one: I don’t know if I like having the rules live in a separate repository. That seems counter to how we have all our other tooling

helen [6:33 AM] i would agree.

First up I'll note that I am an "owner" of both the ​https://github.com/stylelint/stylelint-config-wordpress and ​https://www.npmjs.com/package/stylelint-config-wordpress repos and adding other maintainers such as yourselves Jorbin, Helen, Nacin etc has always been part of my greater plan as this WordPress and Stylelint project moves forward so that timely updates, pull requests, and deployments etc can be made by many and not be stuck waiting on any one individual.

Currently the only way to define Stylelint rules are in said separate repository, there are future plans to add support for a .stylelintrc file, similar to .jshint and the like though this has yet to be implemented.

I personally also see the separate repository as an advantage that allows similar shared benefits as the WordPress PHP Coding Standards project ​here where a "shared" configuration is used by the wider WordPress community as a whole.

[netweb]

Replying to slackbot:

This ticket was mentioned in ​Slack in #core by jorbin. ​View the logs.

jorbin [6:34 AM] Have you given a thorough review to all the rules? Are you ok with them?

The full list of Stylineline rules is here: ​https://github.com/stylelint/stylelint/blob/master/docs/rules.md

helen [6:35 AM] i’m taking a second look right now, always hard to digest all at once

I still am, and have looked at all of these soooo many times ;)

jorbin [6:35 AM] Have we tested to make sure it will catch parse errors in the CSS correctly?

The current count for Stylinelints 87 rules is 7,068 tests

[netweb]

I'll work up a fresh patch that should give us an idea of where we're sitting in relation to both what we're looking at for the tooling side and for the CSS changes.

At the same time, I'll include feedback from both your Helen and Nacin comments comment:42 and comment:40 respectively with some answers and/or stats based on the issues/questions you both raised.

[DrewAPicture]

@netweb Any word on an updated patch per your comment:47? 4.4 Beta1 is looming (Oct. 21).

[netweb]

Rule questions: String Quotes
Replying to nacin:

• string-quotes -- What is more common in our CSS, single quotes or double quotes? We already prefer single quotes in JS (unlike the jQuery project) and in PHP, so should we just keep using it in CSS too? Is there a reason to not be consistent? (I think double quotes look more natural when surrounding a font name, I'm just asking.)

Replying to GaryJ:

Already documented in the CSS standards as: "Use double quotes rather than single quotes, and only when needed, such as when a font name has a space.". Custom fonts, using a single name, don't technically need any quotes, but some folks favour them for some reason.

Replying to helen:

Double quotes somehow "feel" more right in CSS, I think possibly because it's a standard in many other projects. It's not just voodoo personal preference, promise :)

Indeed, defined in our CSS Standards as Use double quotes rather than single quotes, and only when needed, such as when a font name has a space. ​here

Stats: ~280 were changed to double quotes in r34011 with 14 new proposed changes here in this ticket versus 977 if we were to change to single quotes

[netweb]

Rule questions: Leading Zero

Replying to nacin:

• The requirement for a leading 0 for a decimal value (.1s or .25) seems weird. They get minified out anyway. And, it seems, we omit them all over the place. Which is more common in our codebase? What's common in the real world? (This would mean number-leading-zero: always).

Replying to GaryJ:

That's part of the documented ​WP coding standards for CSS: "Use a leading zero for decimal values, including in rgba()."

Replying to helen:

My recollection of why we went this route in our coding standards was also for human readability: . is easy to miss, 0. less so. I'd prefer to add them in where missing.

Per the above, it is listed as one of our standards, a quick straw poll of WP Australia was to also include the leading zero, primarily for readability per Helen's explanation above.

Stats: We would have to remove 399 leading zeros if we changed our standards to not include leading zeros versus adding 185 instances where leading zeros are currently missing.

[netweb]

Rule questions: SCSS Comments
Replying to nacin:

• src/wp-admin/css/colors/_admin.scss:83 uses // as a commenting style. I know this is allowed in SCSS, but do these rules mandate // over /* */? Should we always use /* */, since it's ultimately CSS? I don't see what stylelint rule this corresponds to.

Replying to GaryJ:

// gets removed by default during the SCSS->CSS compiling, so it never makes it into CSS. There's a functional difference at play here, not just a code style preference.

Replying to helen:

Per @GaryJ, they are functionally different, but I'm not sure if/when we would have rules in our very limited use of SCSS that shouldn't be in the unminified compiled CSS.

My patch 29792-css-patch.diff and this specific line src/wp-admin/css/colors/_admin.scss:83 was a hack/test I had added but hadn't removed for the patchto workaround upstream node-sass issue ​here that turns out to be a libsass issue ​here and will ship as part of libsass 3.3 ​https://github.com/sass/libsass/issues/1453

Per the above there is functional differences between both comment types and there shouldn't be any changes to existing SCSS for any comments making this a non-issue for now :)

[netweb]

Rule questions: Parentheses and whitespace

Replying to nacin:

• I noticed the removal of spaces within parentheses. Spacing within parentheses is a thing WP likes to do. Can we keep them? I wonder if enforcing them ends up being less changes. (This would mean function-parentheses-space-inside: always.)

Replying to helen:

Let's please keep them and allow minification to strip them for minified files. As with PHP and JS, it's great for readability, which is extremely important for development files.

This is defined in our CSS handbook as: Do not pad parentheses with spaces ​here with examples:

• background-image: url(images/bg.png);
• text-shadow: 0 -1px 0 rgba(0, 0, 0, 0.5),
• @media all and (max-width: 699px) and (min-width: 520px) { <- Note see addendum below

Per the above the current rule configuration is: "function-parentheses-space-inside": [ 2, "never" ], and the full concise rule definition is ​here

Stats: 394 changes required to remove this whitespace versus 816 changes to include this whitespace.

---

Circling back to comment 11 above:
Replying to nacin:

No thanks. :) I patched CSSLint for this: ​https://github.com/CSSLint/parser-lib/pull/125

That pull request created for CSSLint would make the use of whitespace valid in media queries to coincide with the existing CSSLint rule that disallowed whitespace in media queries, as such both of the following would be valid CSSLint:

• @media all and (max-width: 699px) and (min-width: 520px) {
• @media all and ( max-width: 699px ) and ( min-width: 520px ) {

We can do the same to both allow and disallow whitespace by not defining the function-parentheses-space-inside rule.

This is indeed the case in why the media-query-parentheses-space-inside rule (​definition) has not been defined in the current stylelint-config-wordpress ruleset as Nacin had explicitly created a pull request to allow with and without whitespace in media queries and I was not going to define that rule because that decision had already been made in this ticket ;)

EDIT: My bad, turns out indeed media-query-parentheses-space-inside is currently defined as never i.e "media-query-parentheses-space-inside": [ 2, "never" ],, rule definition ​here

Stats: To change our CSS to include this whitespace will be 262 changes or 74 changes to remove the white space in these media queries

[netweb]

Alignment via Whitespace

Replying to nacin:

Where we should change our CSS to conform to the rules, rather than add ignore rules:

• For @-webkit-keyframes dice-color-change, we don't align things the way we do for @-webkit-keyframes customize-reload. We might as well make them standardized so they ignore the same things.

I haven't changed any of these currently, all I have done is added comments so that Stylelint skips the rules that would cause failures, some examples of these are below, no doubt they are more "readable" using the extra whitespace formatting but at this stage we there are no Stylelint rules defined to allow this.

Do we want to remove the extra whitespace to match currently available rules or have new Stylelint rules added to allow whitespace for alignment and readability?

src/wp-admin/css/customize-controls.css

/* stylelint-disable block-opening-brace-newline-after, block-closing-brace-newline-before */
@-webkit-keyframes dice-color-change {
        0% { color: #d4b146; }
        50% { color: #ef54b0; }
        75% { color: #7190d3; }
        100% { color: #d4b146; }
}

@keyframes dice-color-change {
        0% { color: #d4b146; }
        50% { color: #ef54b0; }
        75% { color: #7190d3; }
        100% { color: #d4b146; }
}
/* stylelint-enable */

src/wp-admin/css/customize-controls.css

/* stylelint-disable block-opening-brace-newline-after, block-opening-brace-space-before, block-closing-brace-newline-before */
@-webkit-keyframes customize-reload {
        0%   { opacity: 0; }
        100% { opacity: 1; }
}

@keyframes customize-reload {
        0%   { opacity: 0; }
        100% { opacity: 1; }
}
/* stylelint-enable */

src/wp-admin/css/nav-menus.css

/* stylelint-disable block-opening-brace-newline-after, number-zero-length-no-unit, block-closing-brace-newline-before */
/* WARNING: The factor of 30px is hardcoded into the nav-menus JavaScript. */
.menu-item-depth-0 { margin-left: 0px; }
.menu-item-depth-1 { margin-left: 30px; }
.menu-item-depth-2 { margin-left: 60px; }
.menu-item-depth-3 { margin-left: 90px; }
.menu-item-depth-4 { margin-left: 120px; }
.menu-item-depth-5 { margin-left: 150px; }
.menu-item-depth-6 { margin-left: 180px; }
.menu-item-depth-7 { margin-left: 210px; }
.menu-item-depth-8 { margin-left: 240px; }
.menu-item-depth-9 { margin-left: 270px; }
.menu-item-depth-10 { margin-left: 300px; }
.menu-item-depth-11 { margin-left: 330px; }

.menu-item-depth-0 .menu-item-transport { margin-left: 0px; }
.menu-item-depth-1 .menu-item-transport { margin-left: -30px; }
.menu-item-depth-2 .menu-item-transport { margin-left: -60px; }
.menu-item-depth-3 .menu-item-transport { margin-left: -90px; }
.menu-item-depth-4 .menu-item-transport { margin-left: -120px; }
.menu-item-depth-5 .menu-item-transport { margin-left: -150px; }
.menu-item-depth-6 .menu-item-transport { margin-left: -180px; }
.menu-item-depth-7 .menu-item-transport { margin-left: -210px; }
.menu-item-depth-8 .menu-item-transport { margin-left: -240px; }
.menu-item-depth-9 .menu-item-transport { margin-left: -270px; }
.menu-item-depth-10 .menu-item-transport { margin-left: -300px; }
.menu-item-depth-11 .menu-item-transport { margin-left: -330px; }

body.menu-max-depth-0 { min-width: 950px !important; }
body.menu-max-depth-1 { min-width: 980px !important; }
body.menu-max-depth-2 { min-width: 1010px !important; }
body.menu-max-depth-3 { min-width: 1040px !important; }
body.menu-max-depth-4 { min-width: 1070px !important; }
body.menu-max-depth-5 { min-width: 1100px !important; }
body.menu-max-depth-6 { min-width: 1130px !important; }
body.menu-max-depth-7 { min-width: 1160px !important; }
body.menu-max-depth-8 { min-width: 1190px !important; }
body.menu-max-depth-9 { min-width: 1220px !important; }
body.menu-max-depth-10 { min-width: 1250px !important; }
body.menu-max-depth-11 { min-width: 1280px !important; }
/* stylelint-enable */

src/wp-admin/css/customize-nav-menus.css

/* stylelint-disable block-opening-brace-newline-after, block-opening-brace-space-before, selector-combinator-space-before, block-closing-brace-newline-before */

/* WARNING: The 20px factor is hard-coded in JS. */
.menu-item-depth-0  { margin-left: 0;     }
.menu-item-depth-1  { margin-left: 20px;  }
.menu-item-depth-2  { margin-left: 40px;  }
.menu-item-depth-3  { margin-left: 60px;  }
.menu-item-depth-4  { margin-left: 80px;  }
.menu-item-depth-5  { margin-left: 100px; }
.menu-item-depth-6  { margin-left: 120px; }
.menu-item-depth-7  { margin-left: 140px; }
.menu-item-depth-8  { margin-left: 160px; } /* Not likely to be used or useful beyond this depth */
.menu-item-depth-9  { margin-left: 180px; }
.menu-item-depth-10 { margin-left: 200px; }
.menu-item-depth-11 { margin-left: 220px; }

/* @todo handle .menu-item-settings width */
.menu-item-depth-0  > .menu-item-bar { margin-right: 0;     }
.menu-item-depth-1  > .menu-item-bar { margin-right: 20px;  }
.menu-item-depth-2  > .menu-item-bar { margin-right: 40px;  }
.menu-item-depth-3  > .menu-item-bar { margin-right: 60px;  }
.menu-item-depth-4  > .menu-item-bar { margin-right: 80px;  }
.menu-item-depth-5  > .menu-item-bar { margin-right: 100px; }
.menu-item-depth-6  > .menu-item-bar { margin-right: 120px; }
.menu-item-depth-7  > .menu-item-bar { margin-right: 140px; }
.menu-item-depth-8  > .menu-item-bar { margin-right: 160px; }
.menu-item-depth-9  > .menu-item-bar { margin-right: 180px; }
.menu-item-depth-10 > .menu-item-bar { margin-right: 200px; }
.menu-item-depth-11 > .menu-item-bar { margin-right: 220px; }

/* Submenu left margin. */
.menu-item-depth-0  .menu-item-transport { margin-left: 0;      }
.menu-item-depth-1  .menu-item-transport { margin-left: -20px;  }
.menu-item-depth-3  .menu-item-transport { margin-left: -60px;  }
.menu-item-depth-4  .menu-item-transport { margin-left: -80px;  }
.menu-item-depth-2  .menu-item-transport { margin-left: -40px;  }
.menu-item-depth-5  .menu-item-transport { margin-left: -100px; }
.menu-item-depth-6  .menu-item-transport { margin-left: -120px; }
.menu-item-depth-7  .menu-item-transport { margin-left: -140px; }
.menu-item-depth-8  .menu-item-transport { margin-left: -160px; }
.menu-item-depth-9  .menu-item-transport { margin-left: -180px; }
.menu-item-depth-10 .menu-item-transport { margin-left: -200px; }
.menu-item-depth-11 .menu-item-transport { margin-left: -220px; }

/* WARNING: The 20px factor is hard-coded in JS. */
.reordering .menu-item-depth-0  { margin-left: 0;     }
.reordering .menu-item-depth-1  { margin-left: 15px;  }
.reordering .menu-item-depth-2  { margin-left: 30px;  }
.reordering .menu-item-depth-3  { margin-left: 45px;  }
.reordering .menu-item-depth-4  { margin-left: 60px;  }
.reordering .menu-item-depth-5  { margin-left: 75px;  }
.reordering .menu-item-depth-6  { margin-left: 90px;  }
.reordering .menu-item-depth-7  { margin-left: 105px; }
.reordering .menu-item-depth-8  { margin-left: 120px; } /* Not likely to be used or useful beyond this depth */
.reordering .menu-item-depth-9  { margin-left: 135px; }
.reordering .menu-item-depth-10 { margin-left: 150px; }
.reordering .menu-item-depth-11 { margin-left: 165px; }

.reordering .menu-item-depth-0  > .menu-item-bar { margin-right: 0;     }
.reordering .menu-item-depth-1  > .menu-item-bar { margin-right: 15px;  }
.reordering .menu-item-depth-2  > .menu-item-bar { margin-right: 30px;  }
.reordering .menu-item-depth-3  > .menu-item-bar { margin-right: 45px;  }
.reordering .menu-item-depth-4  > .menu-item-bar { margin-right: 60px;  }
.reordering .menu-item-depth-5  > .menu-item-bar { margin-right: 75px;  }
.reordering .menu-item-depth-6  > .menu-item-bar { margin-right: 90px;  }
.reordering .menu-item-depth-7  > .menu-item-bar { margin-right: 105px; }
.reordering .menu-item-depth-8  > .menu-item-bar { margin-right: 120px; }
.reordering .menu-item-depth-9  > .menu-item-bar { margin-right: 135px; }
.reordering .menu-item-depth-10 > .menu-item-bar { margin-right: 150px; }
.reordering .menu-item-depth-11 > .menu-item-bar { margin-right: 165px; }
/* stylelint-enable */

[netweb]

Disallow units for zero lengths
Replying to nacin:

Where we should change our CSS to conform to the rules, rather than add ignore rules:

• We shouldn't ignore number-zero-length-no-unit. Remove the units.

Replying to helen:

Patch seems to remove them - did I miss something?

Indeed Disallow units for zero lengths is included in the patch using the number-zero-length-no-unit rule , full definition ​here

[netweb]

Summarising the above comments for feedback from @nacin, @helen, and @jorbin:

[ ] Comment 44: Multiple comma-separated values
[ ] Comment 45: NPM module stylelint-config-wordpress configuration
[ ] Comment 46: Stylelint rule unit tests
[ ] Comment 49: String Quotes
[ ] Comment 50: Leading Zero
[ ] Comment 51: SCSS Comments
[ ] Comment 52: Parentheses and whitespace
[ ] Comment 53: Alignment via Whitespace
[ ] Comment 54: Disallow units for zero lengths

[helen]

Replying to netweb:

First up I'll note that I am an "owner" of both repos

Who "owns" the repository doesn't erase the fact that it's separate AND external. I'm still not very excited about the prospect of having this be external, especially while we're still working things out. Having it external AND used in core makes it "blessed", when it's not something I'd be comfortable calling final for some time yet.

Currently the only way to define Stylelint rules are in said separate repository, there are future plans to add support for a .stylelintrc file, similar to .jshint and the like though this has yet to be implemented.

That's too bad. I would rather punt on this, then.

By the way, you don't need to keep citing the WP CSS Coding Standards as a biblical source - it is a living/evolving document that has very little to do with our long-existing mess of CSS, and I wrote them in the first place. :)

[jorbin]

I think it might make sense to bring this ticket back to just focussing on syntax checking, rather than also focusing on style linting. If we had a tool that could tell developers that a css files lacks an expected semi colon or you are attempting to set the magin property instead of margin, I would support getting that committed quickly. Full on linting for style clearly is bound to get bogged down and take much longer to get committed.

[netweb]

Replying to helen:

Replying to netweb:

First up I'll note that I am an "owner" of both repos

Who "owns" the repository doesn't erase the fact that it's separate AND external. I'm still not very excited about the prospect of having this be external, especially while we're still working things out. Having it external AND used in core makes it "blessed", when it's not something I'd be comfortable calling final for some time yet.

Right, I see your point, see below, maybe in the future we can revisit this and bless it if deserved ;)

Currently the only way to define Stylelint rules are in said separate repository, there are future plans to add support for a .stylelintrc file, similar to .jshint and the like though this has yet to be implemented.

Over the weekend Stylelint 2.0.0 was released with full support for .stylelintrc, patch 29792.3.diff uses this new configuration file rather than the previous "external repo".

That's too bad. I would rather punt on this, then.

I'm updating the CSS patch based on the rules defined in 29792.3.diff and we can go from there on punting decisions

By the way, you don't need to keep citing the WP CSS Coding Standards as a biblical source - it is a living/evolving document that has very little to do with our long-existing mess of CSS, and I wrote them in the first place. :)

I linked to these because I needed to cite "something" for any decisions I had made in regard to the rules I'd chosen was the only reason :)

Edit Note: Patch 29792.3.diff also includes the following patch as these updated dependancies are required 34177-autoprefixer-postcss.diff

Run npm install to update dependancies and run either grunt precommit or grunt postcss:lint to run the Stylelint linter :)

[jorbin]

I tested and stylint doesn't detect syntax errors. It does a great job of detecting formatting errors, but it misses things like argin: 20px; AKA, I can still jorbin my css.

Punting this from 4.4 while work continues.

[helen]

In 37363:

Revive grunt-rtlcss, which does not appear to enjoy syntax errors.

props netweb.
see #36753, #29792.