csscomb / csscomb.js

CSS coding style formatter
http://csscomb.com/
MIT License
3.29k stars 457 forks source link

Lines between rulesets #450

Closed ravasthi closed 8 years ago

ravasthi commented 8 years ago

Add lines-between-rulesets option.

Taking over where #436 left off; apologies that the history looks a bit messy.

@sodatea Please review. Thanks!

haoqunjiang commented 8 years ago

And why not add sass support for this rule? It should be quite trivial to do so.

ravasthi commented 8 years ago

I'll take a stab at sass support and see how it goes.

haoqunjiang commented 8 years ago

Fow now, sass support is buggy. Seems that gonzales-pe returns false for some .is('space'). Maybe it's because they are declarationDelimiter type rather than simple space nodes in sass.

ravasthi commented 8 years ago

Okay! I think I got it working. Please take a look when you get a chance. Thanks!

haoqunjiang commented 8 years ago

Good job! I think this PR is almost good to go. But I've got a small problem with comments:

Source:

.foo {
    background: red;
}
/* comment */
.bar {
    border: 1px solid red;
}

After csscomb:

.foo {
    background: red;
}
/* comment */

.bar {
    border: 1px solid red;
}

Notice the position of the comment block. Not sure if this behavior is acceptable. What do you think?

ravasthi commented 8 years ago

I would consider that behavior unacceptable, given that it's a fair assumption the comment belongs to the block below. It was a bit tricky, but I think I got the code to correctly handle this case for one or more single-line or multi-line comments preceding a ruleset. Please have a look.

ravasthi commented 8 years ago

Hi @sodatea, have you had a chance to review this yet? Please let me know if there's anything further you'd like me to change. Thanks!

haoqunjiang commented 8 years ago

@ravasthi Sorry, I was on vacation for the last few days and the network connection was too poor.

There are still problems with comments in your PR.

Before:

.foo {background: red;}/*comment*/.bar{border: 1px solid red;}

After:

.foo {background: red;}/*comment*/.bar{border: 1px solid red;}
ravasthi commented 8 years ago

This was an easy fix; just one small thing I wasn't checking for. Should be all working now.

haoqunjiang commented 8 years ago

OK. It looks pretty good now.

Finally, just before merging this branch, it would be much appreciated if you could rebase and squash your commits to get a clean commit history.

ravasthi commented 8 years ago

Okay, squashed!

haoqunjiang commented 8 years ago

@ravasthi Thanks!

ravasthi commented 8 years ago

:+1: Looking forward to the next release!

zhe commented 8 years ago

Looking forward to it!

jwhitmarsh commented 8 years ago

Is there any word on this working for Sass?