houndci / hound

Automated code review for GitHub pull requests.
https://houndci.com
MIT License
1.95k stars 401 forks source link

Alphabetical SCSS property ordering breaks progressive enhancement #696

Closed joaocunha closed 9 years ago

joaocunha commented 9 years ago

The sorting shouldn't be alphabetical, as it breaks progressive enhancement/graceful degradation practices. Example:

This (pseudo(ish)code):

.fancy {
  background-color: red;
  background-image: url(http://foo.com/radial.png);
  background-position: 80% 20%;
  background-repeat: no-repeat;
  background: -webkit-gradient(radial, 80% 20%, 0, 80% 40%, 100, from(blue), to(cyan));
}

Is suggested by the tool to be rewritten as:

.fancy {
  background: -webkit-gradient(radial, 80% 20%, 0, 80% 40%, 100, from(blue), to(cyan));
  background-color: red;
  background-image: url(http://foo.com/radial.png);
  background-position: 80% 20%;
  background-repeat: no-repeat;
}

CSSComb is doing a great job on ordering, maybe we could have something similar: https://github.com/csscomb/csscomb.js/blob/master/config/yandex.json

croaky commented 9 years ago

Thanks for the heads up about CSSComb. That's good to know.

I don't write much SCSS, but in this example, would it be more typical to handle the progressive enhancement using a mixin like http://bourbon.io/docs/#radial-gradient? I'm thinking the benefits of the mixin are less code, fewer potential mistakes, and it allows being able to sort the rules alphabetically, which is great for quickly ordering mechanically in a text editor or shell. Sor example, using :sort in Vim.

https://robots.thoughtbot.com/sort-lines-alphabetically-in-vim

joaocunha commented 9 years ago

You're welcome, @croaky.

Mixins can be used, yes, but they are not a default pick for everybody. I personally prefer to write SCSS with a syntax as close as possible to vanilla CSS, handling prefixes with Autoprefixer. It makes it easier to move between pre-processors this way, and the code is more future-oriented.

That being said, it also wouldn't solve the problem. If the mixin is reordered to be above or below a certain point in the code, it could break again.

I feel that ordering should be optional (defaults to false) and configurable.

croaky commented 9 years ago

Gotcha. Thanks, @joaocunha. It's always useful to hear about folks' preferred workflows.

Your preferred sort order should be supported by Hound right now. We use SCSS Lint, which provides this Property Sort Order configuration option.

If you follow the Hound configuration steps and set the Property Sort Order option to your liking, Hound should do the right thing for you.

Hope that helps!

salbertson commented 9 years ago

Thanks @joaocunha! Let us know if you were able to configure Hound the way you wanted.

joaocunha commented 9 years ago

@salbertson I'll give it a try this week most likely, and I'll give my feedback. Thanks for the prompt response!

salbertson commented 9 years ago

@joaocunha, no problem, thank you.

thorncp commented 9 years ago

@joaocunha Any updates? Has this worked out for you?

joaocunha commented 9 years ago

@thorncp yup! Sorry for not getting back in touch!