thoughtbot / bitters

Add a dash of pre-defined style to your Bourbon.
https://thoughtbot.com
MIT License
1.39k stars 142 forks source link

Update bitters to match thoughtbot's style preferences #260

Closed creuter closed 7 years ago

creuter commented 8 years ago

When installing bitters, Hound throws a few comments because it doesn't quite match our style preferences.

$form-box-shadow: inset 0 1px 3px rgba(#000, 0.06);

Color literals like rgba(#000, 0.06) should only be used in variable declarations; they should be referred to via variable everywhere else.

color: #fff;

Color literals like #fff should only be used in variable declarations; they should be referred to via variable everywhere else.

+#{$all-buttons} {

Rule set contains (19/10) properties

whmii commented 8 years ago
  1. I actually disagree with the linter here. #000 is in a variable. open to discussion though.
  2. bourbon 5 introduces a color-switch (or something like that) function. we are biding our time until the full release so that we can just use that instead. in the meantime I can use $base-background-color instead if that is preferable
  3. seems like more of a suggestion than rigid enforcement. If there is one property that would/should have a lot of rules on it, it would be buttons. They have to override a large amount of browser styles. definitely open to changes tho.
tysongach commented 7 years ago

https://github.com/thoughtbot/bitters/pull/294 address the second warning (“Color literals like #fff”), per @whmii’s suggestion.

In regards to the third warning, I think we just need to disable that linter. I agree that this can’t really be a rigid enforcement.

tysongach commented 7 years ago

We disable the PropertyCount linter (which is what throws the warning “Rule set contains (19/10) properties”) in the default scss-lint config our guides. I think that’s a fine solution and I’m not sure we can change much here in Bitters to do anything more.

By default, Hound also uses our default scss-lint config on all thoughtbot repos, so we should be set there. However, if you run scss-lint locally without any configuration, their default config has PropertyCount and set to a max of 10 properties.

tysongach commented 7 years ago

I’m going to close this. As of the latest release (1.7.0), we are now down to just one warning when using the scss-lint config from our guides:

base/_forms.scss:1:36 [W] ColorVariable: Color literals like `rgba(#000, 0.06)` should only be used in variable declarations; they should be referred to via variable everywhere else.

I think this is a failure of the linter. It points to this line which is using a variable and the color that I think it doesn’t like is #000 which is pure black, not some color from a scheme.