Issue 4155 - Created global csscomb config

I created this .csscomb.json 

CSScomb is a coding style formatter for CSS. You can easily write your own configuration to make your style sheets beautiful and consistent.

The main feature is sorting properties in a specific order. It was inspired by @miripiruni's PHP-based tool of the same name. This is the new JavaScript version, based on the powerful CSS parser Gonzales PE.

Find more information about csscomb here:

- http://csscomb.com/
- https://github.com/csscomb/csscomb.js
- https://github.com/csscomb/csscomb.js/blob/dev/doc/options.md

I found this tool very useful because:

- I tend ignore sort-order
- I tend to put brackets on the same line
- I tend to copy/paste from the web and dev tools

And it was really annoying to fix resulting CSS formatting issues manually.

Using this tool, I simply ran `csscomb` from the cli, my editor (via atom plugin), or my build procedure (via gulp) whenever I:

- pasted code
- noticed errors
- uploaded a review

And saved myself (and my reviewer) a ton of time - allowing us to focus on the *real* CSS issues.

I'm not sure whether this tool can/should replace a CSS Linter. But I do think that it is worth sharing. And it **may** be all we need as far as CSS Linting goes (if we want to keep things simple).

[juliandoucette, 2016-06-15 14:05:58]

I will create an issue in Trac for this ASAP (Trac is down).

For those of you who aren't familiar with csscomb: It's a tool that formats CSS.
You can use it with your favorite editor via plugin or integrate it with your
grunt/gulp/whatever build procedure.

This config contains the default "sort-order" called "yandex". I chose to use
this sort order because it is the most similar to ours and I did not think it
would be a productive use of time to create a new one based on ours.

[juliandoucette, 2016-06-15 14:37:58]

On 2016/06/15 14:05:58, juliandoucette wrote:
> I will create an issue in Trac for this ASAP (Trac is down).
> 
> For those of you who aren't familiar with csscomb: It's a tool that formats
CSS.
> You can use it with your favorite editor via plugin or integrate it with your
> grunt/gulp/whatever build procedure.
> 
> This config contains the default "sort-order" called "yandex". I chose to use
> this sort order because it is the most similar to ours and I did not think it
> would be a productive use of time to create a new one based on ours.

Trac issue created: https://issues.adblockplus.org/ticket/4155

[juliandoucette, 2016-09-15 15:57:08]

Updated reviewers based on discussion in last Frontend Developers meeting.

[Wladimir Palant, 2016-09-16 08:18:20]

This config file is quite large, if I see it correctly csscomb doesn't support
inheriting from a predefined configuration. Did you consider using stylelint? I
am using it with a very minimal config, see
https://github.com/palant/easypasswords/blob/67f1fa04b1e470851560e896c68dfa90....
You can see stylelint-config-standard under
https://github.com/stylelint/stylelint-config-standard/blob/master/index.js, it
seems to cover most of the things one would need. Personally, I also prefer
issues to be pointed out rather than auto-corrected - I tend to distrust
automated correction mechanisms.

[kzar, 2016-09-16 10:04:35]

Mind removing me as a reviewer for this one? I don't think I can offer much
help, I'm not great at CSS.

[juliandoucette, 2016-09-16 10:51:57]

On 2016/09/16 10:04:35, kzar wrote:
> Mind removing me as a reviewer for this one? I don't think I can offer much
> help, I'm not great at CSS.

Done.

[juliandoucette, 2016-09-16 12:23:24]

> This config file is quite large, if I see it correctly csscomb doesn't support
> inheriting from a predefined configuration. Did you consider using stylelint?
I
> am using it with a very minimal config, see
>
https://github.com/palant/easypasswords/blob/67f1fa04b1e470851560e896c68dfa90....
> You can see stylelint-config-standard under
> https://github.com/stylelint/stylelint-config-standard/blob/master/index.js,
it
> seems to cover most of the things one would need. Personally, I also prefer
> issues to be pointed out rather than auto-corrected - I tend to distrust
> automated correction mechanisms.

- Yes, it is quite large (mostly because of the custom sort-order definition)
- No, it does not support inheriting from a predefined configuration
- No, I did not consider using stylelint (thanks for the tip)
- But yes, I did consider using other CSSLinters

I chose a code formatter instead of a code linter because:

- I tend to ignore sort-order
- I tend to put opening brackets on the same line
- I often copy CSS from the web and dev tools

And I found it really annoying to fix resulting format issues manually. 

So:

- I used it to auto-format after copy/pasting
- I used it to auto-format after making mistakes repeatedly
- I used it to auto-format before uploading my CSS for review

And I found that it saved me (and my reviewer) a ton of time - allowing us to
focus on *real* CSS issues.

I thought that this tool covered our CSS styleguide effectively, and was worth
sharing, even if we decide to define a linter config later.

[Thomas Greiner, 2016-09-16 12:47:03]

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json
File .csscomb.json (right):

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newcode1
.csscomb.json:1: {
I'd suggest adding "lines-between-rulesets".

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:24: "sort-order": [
Our coding style only cares about the order of groups of properties, not of each
individual property (see
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/...).
We agreed on that to keep the ordering of properties simple but still somewhat
organized.

Therefore let's use CSScomb's other accepted value instead: "{Array} of arrays
of rules for groups separation" (see
https://github.com/csscomb/csscomb.js/blob/dev/doc/options.md#sort-order).

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:38: "-ms-overflow-x",
Vendor-prefixed properties should be overridden by their standardized versions,
not the other way around. This is consistent with how we've been doing it so far
and allows us to remove them later on without breaking anything.

Note that I'm not even sure whether we should include vendor-prefixed properties
in here. There might be situtations where changing the order could be necessary
due to differing behaviors. But I'll leave that up to you.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:208: "filter:progid:DXImageTransform.Microsoft.Alpha(Opacity",
Isn't the property name just "filter"?

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:208: "filter:progid:DXImageTransform.Microsoft.Alpha(Opacity",
I noticed that those IE-specific properties are spread all over while I didn't
find the to-be-standardized "filter" property anywhere (see
https://developer.mozilla.org/en-US/docs/Web/CSS/filter) with which we could
group them together.

[juliandoucette, 2016-09-16 16:32:27]

Thank you Thomas! See comments below.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json
File .csscomb.json (right):

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newcode1
.csscomb.json:1: {
On 2016/09/16 12:47:03, Thomas Greiner wrote:
> I'd suggest adding "lines-between-rulesets".

This property was added after this config was created.

I agree.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:24: "sort-order": [
On 2016/09/16 12:47:02, Thomas Greiner wrote:
> Our coding style only cares about the order of groups of properties, not of
each
> individual property (see
>
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/...).
> We agreed on that to keep the ordering of properties simple but still somewhat
> organized.
> 
> Therefore let's use CSScomb's other accepted value instead: "{Array} of arrays
> of rules for groups separation" (see
> https://github.com/csscomb/csscomb.js/blob/dev/doc/options.md#sort-order).

I think you or I may be misinterpreting what {Array} of array does here. The
**only** thing that {Array} of arrays seems to do is automatically add newlines
between the properties that are separated by array. (That is why I changed the
value from Array:array to Array:string in my last changeset.)

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:38: "-ms-overflow-x",
On 2016/09/16 12:47:02, Thomas Greiner wrote:
> Vendor-prefixed properties should be overridden by their standardized
versions,
> not the other way around. This is consistent with how we've been doing it so
far
> and allows us to remove them later on without breaking anything.
> 
> Note that I'm not even sure whether we should include vendor-prefixed
properties
> in here. There might be situtations where changing the order could be
necessary
> due to differing behaviors. But I'll leave that up to you.

I agree. 

PS: Like I said, this is the default "yandex" config. I chose this config
because it was the closest to ours. I did not change it until my last changeset
where I removed the Array:arrays and display order.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:208: "filter:progid:DXImageTransform.Microsoft.Alpha(Opacity",
On 2016/09/16 12:47:02, Thomas Greiner wrote:
> I noticed that those IE-specific properties are spread all over while I didn't
> find the to-be-standardized "filter" property anywhere (see
> https://developer.mozilla.org/en-US/docs/Web/CSS/filter) with which we could
> group them together.

See the "PS:" from my comment above.

I think that we should probably remove all proprietary properties from this list
- Since we generally discourage their use
- In favour of graceful degradation, by design

[juliandoucette, 2016-09-16 16:43:01]

See a correction and a clarification of my previous comments below.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json
File .csscomb.json (right):

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:38: "-ms-overflow-x",
On 2016/09/16 16:32:27, juliandoucette wrote:
> On 2016/09/16 12:47:02, Thomas Greiner wrote:
> > Vendor-prefixed properties should be overridden by their standardized
> versions,
> > not the other way around. This is consistent with how we've been doing it so
> far
> > and allows us to remove them later on without breaking anything.
> > 
> > Note that I'm not even sure whether we should include vendor-prefixed
> properties
> > in here. There might be situtations where changing the order could be
> necessary
> > due to differing behaviors. But I'll leave that up to you.
> 
> I agree. 
> 
> PS: Like I said, this is the default "yandex" config. I chose this config
> because it was the closest to ours. I did not change it until my last
changeset
> where I removed the Array:arrays and display order. 

To clarify: 

I'm talking about the sort-order value. Not the whole config.

See: https://github.com/csscomb/csscomb.js/tree/dev/config

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:208: "filter:progid:DXImageTransform.Microsoft.Alpha(Opacity",
On 2016/09/16 16:32:27, juliandoucette wrote:
> On 2016/09/16 12:47:02, Thomas Greiner wrote:
> > I noticed that those IE-specific properties are spread all over while I
didn't
> > find the to-be-standardized "filter" property anywhere (see
> > https://developer.mozilla.org/en-US/docs/Web/CSS/filter) with which we could
> > group them together.
> 
> See the "PS:" from my comment above.
> 
> I think that we should probably remove all proprietary properties from this
list
> - Since we generally discourage their use
> - In favour of graceful degradation, by design

In favour of progressive enhancement, by design*

[Thomas Greiner, 2016-09-16 17:39:30]

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json
File .csscomb.json (right):

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:24: "sort-order": [
On 2016/09/16 16:32:27, juliandoucette wrote:
> On 2016/09/16 12:47:02, Thomas Greiner wrote:
> > Our coding style only cares about the order of groups of properties, not of
> each
> > individual property (see
> >
>
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/...).
> > We agreed on that to keep the ordering of properties simple but still
somewhat
> > organized.
> > 
> > Therefore let's use CSScomb's other accepted value instead: "{Array} of
arrays
> > of rules for groups separation" (see
> > https://github.com/csscomb/csscomb.js/blob/dev/doc/options.md#sort-order).
> 
> I think you or I may be misinterpreting what {Array} of array does here. The
> **only** thing that {Array} of arrays seems to do is automatically add
newlines
> between the properties that are separated by array. (That is why I changed the
> value from Array:array to Array:string in my last changeset.)

Ah, I see, my bad.

Regarding using a linter or a beautifier, I'd prefer educating the developer on
how code should look like with the help of a linter to avoid dependence on
specific tools. This might especially be useful if you want to contribute to
other projects where you don't have such tools in place.

Anyway, if you still think that a beautifier provides enough value, maybe best
to open a separate discussion since such a decision might affect other
developers as well in the future.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:38: "-ms-overflow-x",
On 2016/09/16 16:32:27, juliandoucette wrote:
> On 2016/09/16 12:47:02, Thomas Greiner wrote:
> > Vendor-prefixed properties should be overridden by their standardized
> versions,
> > not the other way around. This is consistent with how we've been doing it so
> far
> > and allows us to remove them later on without breaking anything.
> > 
> > Note that I'm not even sure whether we should include vendor-prefixed
> properties
> > in here. There might be situtations where changing the order could be
> necessary
> > due to differing behaviors. But I'll leave that up to you.
> 
> I agree. 
> 
> PS: Like I said, this is the default "yandex" config. I chose this config
> because it was the closest to ours. I did not change it until my last
changeset
> where I removed the Array:arrays and display order. 

Yes, but I don't think we should enforce something yet that doesn't match our
coding style. That could easily lead to confusion and inconsistencies.

So thanks for making the changes.

Generally, if you want to tackle it quickly and iterate on it over time, I guess
starting from nothing and building it up piece by piece appears more reliable
than doing it the other way around.

https://codereview.adblockplus.org/29346536/diff/29353262/.csscomb.json#newco...
.csscomb.json:208: "filter:progid:DXImageTransform.Microsoft.Alpha(Opacity",
On 2016/09/16 16:43:01, juliandoucette wrote:
> On 2016/09/16 16:32:27, juliandoucette wrote:
> > On 2016/09/16 12:47:02, Thomas Greiner wrote:
> > > I noticed that those IE-specific properties are spread all over while I
> didn't
> > > find the to-be-standardized "filter" property anywhere (see
> > > https://developer.mozilla.org/en-US/docs/Web/CSS/filter) with which we
could
> > > group them together.
> > 
> > See the "PS:" from my comment above.
> > 
> > I think that we should probably remove all proprietary properties from this
> list
> > - Since we generally discourage their use
> > - In favour of graceful degradation, by design
> 
> In favour of progressive enhancement, by design*

Agreed.

[juliandoucette, 2016-09-16 19:00:51]

> Regarding using a linter or a beautifier, I'd prefer educating the developer
on
> how code should look like with the help of a linter to avoid dependence on
> specific tools. 

Yes, this is one reason why a linter is a good idea.

> This might especially be useful if you want to contribute to
> other projects where you don't have such tools in place.

(If we added this config to codingtools then we would always have this tool in
place.)

> Anyway, if you still think that a beautifier provides enough value, maybe best
> to open a separate discussion since such a decision might affect other
> developers as well in the future.

I'm confused about where you want me to open a separate discussion and why?

[juliandoucette, 2016-09-16 21:15:32]

- I updated the description to describe what CSSComb is and why I decided to
share my config
- I removed all of the proprietary and unsupported CSS properties from
sort-order (draft)
- I re-ordered and re-grouped (separated by newlines) sort-order properties
according to our style guide (draft)
- I added lines-between-rules

[Thomas Greiner, 2016-09-19 11:57:11]

> If we added this config to codingtools then we would always have this tool in
place.

No, we'd only have it for our own projects. However, there are also other
projects you might be working on that we don't own (e.g. open source projects)
or projects you are/will be working on for other/future employers.

On 2016/09/16 21:15:32, juliandoucette wrote:
> - I updated the description to describe what CSSComb is and why I decided to
> share my config
> - I removed all of the proprietary and unsupported CSS properties from
> sort-order (draft)
> - I re-ordered and re-grouped (separated by newlines) sort-order properties
> according to our style guide (draft)
> - I added lines-between-rules

Just two minor things I noticed but apart from that LGTM if we decide to go with
this approach instead of a linter.

https://codereview.adblockplus.org/29346536/diff/29353362/.csscomb.json
File .csscomb.json (right):

https://codereview.adblockplus.org/29346536/diff/29353362/.csscomb.json#newco...
.csscomb.json:38: "flex-direction",
Detail: What about the "flex" shorthand property?

https://codereview.adblockplus.org/29346536/diff/29353362/.csscomb.json#newco...
.csscomb.json:106: "box-shadow",
Detail: Duplicated values.

[juliandoucette, 2016-10-13 12:15:16]

Thanks Thomas.

Let's finish this review.

Please comment LGTM if you want this in coding tools and Not LGTM if you don't.

https://codereview.adblockplus.org/29346536/diff/29353362/.csscomb.json
File .csscomb.json (right):

https://codereview.adblockplus.org/29346536/diff/29353362/.csscomb.json#newco...
.csscomb.json:38: "flex-direction",
On 2016/09/19 11:57:11, Thomas Greiner wrote:
> Detail: What about the "flex" shorthand property?

Done.

https://codereview.adblockplus.org/29346536/diff/29353362/.csscomb.json#newco...
.csscomb.json:106: "box-shadow",
On 2016/09/19 11:57:11, Thomas Greiner wrote:
> Detail: Duplicated values.

Done.

[juliandoucette, 2016-10-13 14:19:06]

Worth pointing out:

- [stylelint](https://github.com/stylelint/stylelint) covers more CSS rules
- [stylefmt](https://github.com/morishitter/stylefmt) supports stylelint config

[juliandoucette, 2017-01-30 10:49:38]

I closed this ticket/review because:

- stylelint covers more relevant rules
- stylelint handles sass/scss better (which was not a requirement when this
ticket was created)
- csscomb seems less configurable as a linter (which is useful for CI)
- stylefmt covers formatting for stylelint
- stylelint is more actively maintained
- stylelint seems to have better editor plugins