Skip to content

Conversation

@rnsloan
Copy link
Contributor

@rnsloan rnsloan commented Nov 18, 2015

This is useful for when adding new features to the site you do not have to scan around the .styl files to see if an appropriate color already exists

  • store all colors as variables
  • replace #f2f2f2 usage with #f0f0f0 as the two shades are almost identical
  • remove redundant border-color property for the site menu active triangle

screen shot 2015-11-19 at 02 45 39

The variable naming is not imaginative but, having worked on a code base that had 25+ shades of grey, coming up with names like light-gray, lighter-gray, lightest-gray, border-gray3 becomes tiresome and has little value over time. Having said that I have left some of the existing variable names as people may be familiar with them.

- store all colors as variables
- replaced #f2f2f2 usage with #f0f0f0 as the two shades of gray are almost identical
- remove redundant border-color property for the site menu active arrow
@ghost
Copy link

ghost commented Nov 18, 2015

+1

@bnb
Copy link
Contributor

bnb commented Nov 18, 2015

Do we really need 8 grays? I understand a brand gray, a dark gray, and a light gray, but is there a need for five additional grays?

@phillipj
Copy link
Member

Do we really need 8 grays?

Spot on.

@stevemao
Copy link
Contributor

$gray1 = #ccc
$gray2 = #c0c0c0
$gray3 = #ddd
$gray4 = #eee
$gray5 = #f0f0f0

don't really like the variable names.

@rnsloan
Copy link
Contributor Author

rnsloan commented Nov 18, 2015

don't really like the variable names.

Other suggestions are welcome. I would not recommend this variable naming anywhere else, but here are some of the problems I have encountered specifically with css color variable naming.

I can alleviate these somewhat by reducing the number of grays down from 8, but you still run into the same problems even with only a few variants of the same color.

  • Say you have three hues of gray light-gray, lighter-gray, lightest-gray . What do you do when you need to add a fourth gray that is even lighter? lightest-gray2? , even-lightest-gray? What about if you need a gray that is somewhere between light-gray and lighter-gray? Do you refactor the other variable names to fit it in? If you don't then the naming is no longer accurate.
  • Take a different approach and name the color variables related to the html elements or css properties they are for: gray-download-table, gray-border. What do you do if you want to use the same color but for a different element? You have to rethink the naming convention used. In some projects I have worked on I found that it is not renamed; so you have a color called border-gray used in places that are not borders.

@phillipj
Copy link
Member

This raises the need for some CSS cleanup which we haven't addressed yet, since we until recently, have been waiting for things to settle. We have probably ended up with so many grays because no one has taken a look across all pages and decided (with a strict mindset) which grays we really need. I would be much more positive to a strict refactor of colors, rather than encourage more colors to be added.

@rnsloan
Copy link
Contributor Author

rnsloan commented Nov 19, 2015

The number of blacks and grays used on the site has dropped from 12 to 6 (though in the file diff it looks like an increase as many of them were not defined as variables).

If you feel this can be trimmed further please submit a pull request after this.

@phillipj
Copy link
Member

The number of blacks and grays used on the site has dropped from 12 to 6

Sounds like a good effort in the right direction! LGTM now 👍

@bnb
Copy link
Contributor

bnb commented Nov 19, 2015

LGTM. :shipit:

@stevemao
Copy link
Contributor

Definitely looks better 👍

bnb pushed a commit that referenced this pull request Nov 19, 2015
@bnb bnb merged commit 1477661 into nodejs:master Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants