Merge branch 'css-style-guide' into 'master'

CSS style guide

Working towards what was discussed in #13552, this adds the [SCSS Linter gem](https://github.com/brigade/scss-lint) for style guide conformity in CI.

TODO:
- [x] Agree on and write SCSS Style Guide Documentation.
- [x] Document the `scss-lint` config file.
- [x] Figure out how best to run this in CI, right now it's taking longer than I would think it should.
- [x] Use CSSComb for auto-correction (Maybe just include a CSSComb config file and have developers run the node package manually if they're interested in using it?).

My logic for not using CSSComb in the first place is that, AFAIK, we don't currently require Node/NPM, even in the dev environment. Maybe I'm wrong about that? `scss-lint` is a Ruby implementation of a similar concept, but it doesn't include autocorrect.

Is there a way we can run get GitLab CI to run `scss-lint` without having to put together the full GitLab application? Seeing as it's just static analysis, it seems like a waste of time/resources.

/cc @jschatz1 @rspeicher

See merge request !3069

8 files changed, 393 insertions, 0 deletions

diff --git a/.csscomb.json b/.csscomb.json
new file mode 100644
index 00000000000..e353e6a63d0
--- /dev/null
+++ b/.csscomb.json
@@ -0,0 +1,16 @@
+{
+    "always-semicolon": true,
+    "color-case": "lower",
+    "block-indent": "  ",
+    "color-shorthand": true,
+    "element-case": "lower",
+    "space-before-colon": "",
+    "space-after-colon": " ",
+    "space-before-combinator": " ",
+    "space-after-combinator": " ",
+    "space-between-declarations": "\n",
+    "space-before-opening-brace": " ",
+    "space-after-opening-brace": "\n",
+    "space-before-closing-brace": "\n",
+    "unitless-zero": true
+}
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index b4dad768fa4..2ad63548d78 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -122,6 +122,14 @@ rubocop:
     - ruby
     - mysql
 
+scss-lint:
+  stage: test
+  script:
+    - bundle exec rake scss_lint
+  tags:
+    - ruby
+  allow_failure: true
+
 brakeman:
   stage: test
   script:
diff --git a/.scss-lint.yml b/.scss-lint.yml
new file mode 100644
index 00000000000..e350b2073c3
--- /dev/null
+++ b/.scss-lint.yml
@@ -0,0 +1,158 @@
+# Linter Documentation:
+# https://github.com/brigade/scss-lint/blob/master/lib/scss_lint/linter/README.md
+
+scss_files: 'app/assets/stylesheets/**/*.scss'
+
+exclude:
+  - 'app/assets/stylesheets/pages/emojis.scss'
+
+linters:
+  BangFormat:
+    enabled: false
+  
+  BorderZero:
+    enabled: false
+  
+  ColorKeyword:
+    enabled: false
+  
+  ColorVariable:
+    enabled: false
+  
+  Comment:
+    enabled: false
+  
+  DeclarationOrder:
+    enabled: false
+
+  # `scss-lint:disable` control comments should be preceded by a comment
+  # explaining why these linters are being disabled for this file.
+  # See https://github.com/brigade/scss-lint#disabling-linters-via-source for
+  # more information.
+  DisableLinterReason:
+    enabled: true
+  
+  DuplicateProperty:
+    enabled: false
+  
+  EmptyLineBetweenBlocks:
+    enabled: false
+  
+  EmptyRule:
+    enabled: false
+  
+  FinalNewline:
+    enabled: false
+  
+  # HEX colors should use three-character values where possible.
+  HexLength:
+    enabled: true
+  
+  # HEX color values should use lower-case colors to differentiate between
+  # letters and numbers, e.g. `#E3E3E3` vs. `#e3e3e3`.
+  HexNotation:
+    enabled: true
+  
+  IdSelector:
+    enabled: false
+  
+  ImportPath:
+    enabled: false
+  
+  ImportantRule:
+    enabled: false
+  
+  # Indentation should always be done in increments of 2 spaces.
+  Indentation:
+    enabled: true
+    width: 2
+  
+  LeadingZero:
+    enabled: false
+  
+  MergeableSelector:
+    enabled: false
+  
+  NameFormat:
+    enabled: false
+  
+  NestingDepth:
+    enabled: false
+
+  PlaceholderInExtend:
+    enabled: false
+
+  PropertySortOrder:
+    enabled: false
+  
+  PropertySpelling:
+    enabled: false
+  
+  PseudoElement:
+    enabled: false
+  
+  QualifyingElement:
+    enabled: false
+  
+  SelectorDepth:
+    enabled: false
+  
+  # Selectors should always use hyphenated-lowercase, rather than camelCase or
+  # snake_case.
+  SelectorFormat:
+    enabled: true
+    convention: hyphenated_lowercase
+  
+  # Prefer the shortest shorthand form possible for properties that support it.
+  Shorthand:
+    enabled: true
+  
+  # Each property should have its own line, except in the special case of
+  # single line rulesets.
+  SingleLinePerProperty:
+    enabled: true
+    allow_single_line_rule_sets: true
+
+  SingleLinePerSelector:
+    enabled: false
+  
+  SpaceAfterComma:
+    enabled: false
+  
+  # Properties should be formatted with a single space separating the colon
+  # from the property's value.
+  SpaceAfterPropertyColon:
+    enabled: true
+
+  # Properties should be formatted with no space between the name and the
+  # colon.
+  SpaceAfterPropertyName:
+    enabled: true
+  
+  SpaceAroundOperator:
+    enabled: false
+  
+  SpaceBeforeBrace:
+    enabled: false
+  
+  StringQuotes:
+    enabled: false
+
+  TrailingSemicolon:
+    enabled: false
+
+  TrailingWhitespace:
+    enabled: false
+
+  UnnecessaryMantissa:
+    enabled: false
+
+  UnnecessaryParentReference:
+    enabled: false
+
+  VendorPrefix:
+    enabled: false
+
+  # Omit length units on zero values, e.g. `0px` vs. `0`.
+  ZeroUnit:
+    enabled: true
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 30c97429040..7540fa1afcc 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -427,6 +427,7 @@ merge request:
 1.  [Rails](https://github.com/bbatsov/rails-style-guide)
 1.  [Testing](https://github.com/thoughtbot/guides/tree/master/style/testing)
 1.  [CoffeeScript](https://github.com/thoughtbot/guides/tree/master/style/coffeescript)
+1.  [SCSS styleguide][scss-styleguide]
 1.  [Shell commands](doc/development/shell_commands.md) created by GitLab
     contributors to enhance security
 1.  [Database Migrations](doc/development/migration_style_guide.md)
@@ -494,6 +495,7 @@ available at [http://contributor-covenant.org/version/1/1/0/](http://contributor
 [rss-source]: https://github.com/bbatsov/ruby-style-guide/blob/master/README.md#source-code-layout
 [rss-naming]: https://github.com/bbatsov/ruby-style-guide/blob/master/README.md#naming
 [doc-styleguide]: doc/development/doc_styleguide.md "Documentation styleguide"
+[scss-styleguide]: doc/development/scss_styleguide.md "SCSS styleguide"
 [gitlab-design]: https://gitlab.com/gitlab-org/gitlab-design
 [free Antetype viewer (Mac OSX only)]: https://itunes.apple.com/us/app/antetype-viewer/id824152298?mt=12
 [`gitlab1.atype` file]: https://gitlab.com/gitlab-org/gitlab-design/tree/master/gitlab1.atype/
diff --git a/Gemfile b/Gemfile
index 2f14b9965b2..a0e8e796627 100644
--- a/Gemfile
+++ b/Gemfile
@@ -283,6 +283,7 @@ group :development, :test do
   gem 'spring-commands-teaspoon', '~> 0.0.2'
 
   gem 'rubocop', '~> 0.35.0', require: false
+  gem 'scss_lint', '~> 0.47.0', require: false
   gem 'coveralls',  '~> 0.8.2', require: false
   gem 'simplecov', '~> 0.10.0', require: false
   gem 'flog', require: false
diff --git a/Gemfile.lock b/Gemfile.lock
index 14b03bab5dd..f4f5649eb75 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -717,6 +717,9 @@ GEM
     sawyer (0.6.0)
       addressable (~> 2.3.5)
       faraday (~> 0.8, < 0.10)
+    scss_lint (0.47.1)
+      rake (>= 0.9, < 11)
+      sass (~> 3.4.15)
     sdoc (0.3.20)
       json (>= 1.1.3)
       rdoc (~> 3.10)
@@ -1007,6 +1010,7 @@ DEPENDENCIES
   ruby-fogbugz (~> 0.2.1)
   sanitize (~> 2.0)
   sass-rails (~> 5.0.0)
+  scss_lint (~> 0.47.0)
   sdoc (~> 0.3.20)
   seed-fu (~> 2.3.5)
   select2-rails (~> 3.5.9)
diff --git a/doc/development/scss_styleguide.md b/doc/development/scss_styleguide.md
new file mode 100644
index 00000000000..6c48c25448b
--- /dev/null
+++ b/doc/development/scss_styleguide.md
@@ -0,0 +1,194 @@
+# SCSS styleguide
+
+This style guide recommends best practices for SCSS to make styles easy to read,
+easy to maintain, and performant for the end-user.
+
+## Rules
+
+### Naming
+
+CSS classes should use the `lowercase-hyphenated` format rather than
+`snake_case` or `camelCase`.
+
+```scss
+// Bad
+.class_name {
+  color: #fff;
+}
+
+// Bad
+.className {
+  color: #fff;
+}
+
+// Good
+.class-name {
+  color: #fff;
+}
+```
+
+### Formatting
+
+You should always use a space before a brace, braces should be on the same
+line, each property should each get its own line, and there should be a space
+between the property and its value.
+
+```scss
+// Bad
+.container-item { 
+  width: 100px; height: 100px;
+  margin-top: 0;
+}
+
+// Bad
+.container-item
+{
+  width: 100px;
+  height: 100px;
+  margin-top: 0;
+}
+
+// Bad
+.container-item{
+  width:100px;
+  height:100px;
+  margin-top:0;
+}
+
+// Good
+.container-item {
+  width: 100px;
+  height: 100px;
+  margin-top: 0;
+}
+```
+
+Note that there is an exception for single-line rulesets, although these are 
+not typically recommended.
+
+```scss
+p { margin: 0; padding: 0; }
+```
+
+### Colors
+
+HEX (hexadecimal) colors short-form should use shortform where possible, and 
+should use lower case letters to differenciate between letters and numbers, e.
+g. `#E3E3E3` vs. `#e3e3e3`.
+
+```scss
+// Bad
+p {
+  color: #ffffff;
+}
+
+// Bad
+p {
+  color: #FFFFFF;
+}
+
+// Good
+p {
+  color: #fff;
+}
+```
+
+### Indentation
+
+Indentation should always use two spaces for each indentation level.
+
+```scss
+// Bad, four spaces
+p {
+    color: #f00;
+}
+
+// Good
+p {
+  color: #f00;
+}
+```
+
+### Semicolons
+
+Always include semicolons after every property. When the stylesheets are 
+minified, the semicolons will be removed automatically.
+
+```scss
+// Bad
+.container-item {
+  width: 100px;
+  height: 100px
+}
+
+// Good
+.container-item {
+  width: 100px;
+  height: 100px;
+}
+```
+
+### Shorthand
+
+The shorthand form should be used for properties that support it.
+
+```scss
+// Bad
+margin: 10px 15px 10px 15px;
+padding: 10px 10px 10px 10px;
+
+// Good
+margin: 10px 15px;
+padding: 10px;
+```
+
+### Zero Units
+
+Omit length units on zero values, they're unnecessary and not including them 
+is slightly more performant.
+
+```scss
+// Bad
+.item-with-padding {
+  padding: 0px;
+}
+
+// Good
+.item-with-padding {
+  padding: 0;
+}
+```
+
+### Selectors with a `js-` Prefix
+Do not use any selector prefixed with `js-` for styling purposes. These 
+selectors are intended for use only with JavaScript to allow for removal or 
+renaming without breaking styling.
+
+## Linting
+
+We use [SCSS Lint][scss-lint] to check for style guide conformity. It uses the 
+ruleset in `.scss-lint.yml`, which is located in the home directory of the 
+project.
+
+To check if any warnings will be produced by your changes, you can run `rake 
+scss_lint` in the GitLab directory. SCSS Lint will also run in GitLab CI to 
+catch any warnings.
+
+If the Rake task is throwing warnings you don't understand, SCSS Lint's 
+documentation includes [a full list of their linters][scss-lint-documentation].
+
+### Fixing issues
+
+If you want to automate changing a large portion of the codebase to conform to 
+the SCSS style guide, you can use [CSSComb][csscomb]. First install
+[Node][node] and [NPM][npm], then run `npm install csscomb -g` to install 
+CSSComb globally (system-wide). Run it in the GitLab directory with 
+`csscomb app/assets/stylesheets` to automatically fix issues with CSS/SCSS.
+
+Note that this won't fix every problem, but it should fix a majority.
+
+[csscomb]: https://github.com/csscomb/csscomb.js
+[node]: https://github.com/nodejs/node
+[npm]: https://www.npmjs.com/
+[scss-lint]: https://github.com/brigade/scss-lint
+[scss-lint-documentation]: https://github.com/brigade/scss-lint/blob/master/lib/scss_lint/linter/README.md
diff --git a/lib/tasks/scss-lint.rake b/lib/tasks/scss-lint.rake
new file mode 100644
index 00000000000..250fd8699e4
--- /dev/null
+++ b/lib/tasks/scss-lint.rake
@@ -0,0 +1,10 @@
+unless Rails.env.production?
+  require 'scss_lint/rake_task'
+
+  SCSSLint::RakeTask.new do |t|
+    t.config = '.scss-lint.yml'
+    # See https://github.com/brigade/scss-lint/issues/726
+    # Hack, otherwise linter won't respect scss_files option in config file.
+    t.files = []
+  end
+end