Clean up CSS according CSS linter

https://gitlab.com/gitlab-org/gitlab-ce/builds/868898

Our css needs cleaning up in order to pass this style guide. We need to go through each complaint and fix each one.

cc @rspeicher @iamphill @alfredo1 @elvongray

Status changed to closed

Status changed to reopened

mentioned in issue #13552 (closed)

I would think that if the merge happened all at once (with no other commits from other MR's) it would be painless. Most of the changes are relatively minimal. FFF to fff is not cause for conflict as I understand it. But as @rspeicher and I were just saying we want to keep our commits down to less than 200 changes. The smaller the better. I don't think it matters how you divide it up, just that you do divide it up.

SCSS Lint rules to fix:

• HEX Formatting - HEX Color values should use lowercase always and shorthand where possible, e.g. '#fff'.
• Indentation - Use two spaces per indentation level.
• Shorthand - Use shorthand values for margin, padding, etc.
• SelectorFormat - Selectors (class and id names) should always use hyphenated-lowercase, rather than camelCase or snake_case.
• SpaceAfterPropertyColon - Properties should be formatted with a single space separating the colon from the property's value.
• ZeroUnit - Omit length units on zero values, e.g. 0px vs. 0.

Note that if you want a line or set of lines to be ignored by the Linter, you can use // scss-lint:disable RuleName (more info):

// This lint rule is disabled because the class name comes from a gem.
// scss-lint:disable SelectorFormat
.ui_charcoal {
  background-color: #333;
}
// scss-lint:enable SelectorFormat

You'll need to make sure to add a comment on the line above the disable, otherwise the linter will throw a warning because we enable the rule DisableLinterReason. This is to make sure we aren't just ignoring the style guide for no reason, and that others know why the rule is disabled in this specific instance.

mentioned in merge request !3238 (merged)

@jschatz1 the HEX value commit changes 242 lines, I think that'll be the largest rule fix in this list.

@connorshea Just curious: did you do this by hand or did you use the automated tool?

@jschatz1 Automated tool, although I did review them for correctness.

Just wondering. I went through it myself but this sort of thing is prone to error. At least we have a tool and linter now. The key is not to make anything a completely different color than it was before.

Everything is the same from what I can tell. #c66 is equivalent to #cc6666, and HEX values are case-insensitive.

I should probably pull @dzaporozhets into this before we go too much further since he'll want to know.

@rspeicher thanks

@jschatz1 I like the style improvements proposed in https://gitlab.com/gitlab-org/gitlab-ce/issues/14299#note_4274973.

@connorshea please have a separate merge request per rule so its easier to review and accept your contribution.

mentioned in commit e03d0b28

mentioned in commit 3b95d59b

@dzaporozhets can do

mentioned in merge request !3259 (merged)

mentioned in merge request !3260 (merged)

mentioned in commit e106597e

mentioned in commit 6dad8640

mentioned in commit 5d3cd7fd

mentioned in commit 5e7622d2

@jschatz1 would it be alright if I just disable the SelectorFormat rule for now? That one's the vast majority of our lint errors and already an MR has slipped through adding a ZeroUnit warning to the linter (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3257/diffs#f58255ca2668075c771607234d27ac97398deba2_36_40), so I'd like to get it failing builds sooner rather than later. Otherwise I'll be here forever :)

Good idea @connorshea

mentioned in merge request !3309 (merged)

mentioned in merge request !3310 (merged)

mentioned in merge request !3312 (merged)

mentioned in merge request !3313 (merged)

@jschatz1 opened a bunch of MRs fixing the remaining lints. Only 4 left after all these are merged, and all were introduced after I fixed the lints for the SpaceAfterPropertyColon rule :)

Assuming you approve of the changes, is it alright for me to make a single MR that fixes those last few lints and also enables the CI test?

@connorshea great! I'll take a look at them all! Once they are all in I'll leave it up to @rspeicher what he wants to do with the tests.

mentioned in commit 5640249d

mentioned in commit d19abe6f

mentioned in commit a2484ae0

Assuming you approve of the changes, is it alright for me to make a single MR that fixes those last few lints and also enables the CI test?

Works for me.

mentioned in commit 5a1572a4

mentioned in commit e210438e

mentioned in commit 86a4449d

mentioned in commit be039601

mentioned in commit 6e46c56b

mentioned in commit 9684d7fd

mentioned in commit 8d7ad535

mentioned in commit 57dd206f

mentioned in merge request !3353 (merged)

mentioned in commit edac38c4

mentioned in commit 672c8fd0

@jschatz1 I think this can be closed now?

@connorshea looks like it. Good job!

Status changed to closed

Mentioned in commit pfjason/gitlab-ce@6dad8640

Mentioned in commit maxraab/gitlab-ce@6e46c56b

Mentioned in commit maxraab/gitlab-ce@9684d7fd