Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v4: Colors redux #22836

Merged
merged 37 commits into from
Jun 30, 2017
Merged

v4: Colors redux #22836

merged 37 commits into from
Jun 30, 2017

Conversation

mdo
Copy link
Member

@mdo mdo commented Jun 15, 2017

This PR supersedes #22551 with a slightly different approach to enabling more color usage while continuing to support the theme approach that the $brand- variables enable.

New colors

I've updated all the color variables, though I'm still not 100% happy with them. I'm struggling to find the right way to create a legit palette here. However, it's getting closer thanks to Material, Open Color, and more. My intent here is that you can swap all the variables easily with other common color palettes from major systems (iOS, Android, etc). As such it's important to have the right number of colors in place to build on.

screen shot 2017-06-15 at 11 18 54 am

Dual Sass color maps

In #22551 I was pushing heavily towards a single Sass map, but not everyone builds on Bootstrap with Sass, so that limits options for those adding vanilla CSS on top of our code. As such, it's entirely possible .btn-blue could become a purple button.

Now, we have two Sass maps, $colors and $theme-colors, though they're not entirely implemented right now. $colors is for every color. $theme-colors is for a subset of those that you'd want to loop over to generate a customized color scheme. The intent is to use this theme Sass map to generate our alerts, buttons, backgrounds, etc.

screen shot 2017-06-15 at 11 23 35 am

Grayscale

In addition to the two color maps, I've added a $grays Sass map and new variables. Instead of a few $gray-light, $gray-dark, etc variables, we have $gray-100 through $gray-900. This provides greater flexibility, a consistent set of grays throughout, and the basis for future color updates. (Eventually every color should have all these shades.)

screen shot 2017-06-15 at 11 27 19 am

Additional improvements

I'm after a number of things with a color update like this. First, I want more colors available to folks to pull from and more options to customize via CSS or Sass (our preferred method). Second, I want the tooling to be helpful, so color contrast functions and proper map values is important. Third, I want to provide more utilities for folks based on these colors (e.g., border color classes) while reducing component-based modifiers where appropriate (e.g., badge backgrounds can be .bg- classes).

Todo

  • Finalize color palette. Please, any and all suggestions on how to build one well is welcome!
  • Determine what needs to be in the $theme-colors map. I've added foreground/background, but are there more? Should it be fewer?
  • Ensure we can easily add additional Sass maps for color shades in a future release.
  • Ensure map-get functions are in place for each Sass map (think I'm missing grays right now)
  • Improve customization docs.

@mdo mdo mentioned this pull request Jun 15, 2017
4 tasks
@Thomas--S
Copy link

This PR seems to be a very good approach to me. It unites the good things from the current system and the closed PR. Thanks!

My suggestions regarding the color palette:

  • Rename blue and indigo to light blue and dark blue
  • Split green into light green (e.g. #00dd00) and dark green (e.g. #008700)
  • Maybe add a brown color (e.g. #8b4513)

@bbmatt
Copy link

bbmatt commented Jun 15, 2017

This seems a very logical approach, although I wonder whether the concept of percentages could be used rather than 100 through to 900?
From a design perspective, color percentage values are widely used - 10% grey, 20% grey etc.?

@IamManchanda
Copy link

@mdo This is a very good approach compared to old one! 👍

mdo added 2 commits June 18, 2017 02:19
background-color: $active-background;
border-color: $active-border;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line contains trailing whitespace

@roooodcastro
Copy link
Contributor

I think there should be a way to overwrite the way borders, shadows, hover, focus, etc are defined, fixing #21037.

#21670 defines variables for basically everything. I think adding those (or at least some) variables in this $theme-colors map would be a good solution.

@wilsonge
Copy link

Looks much better than the last approach. Thanks for listening to the concerns :)

@gseregni
Copy link

gseregni commented Jun 22, 2017

@mdo IMHO a good starting point for $theme-colors map would be to include primary-secondary-faded-inverse-info-success-warning-danger. I can't understand background-foreground meaning.

BTW color-yiq mixin and $theme-colors are super-usefull (Claps hands)

@mdo
Copy link
Member Author

mdo commented Jun 22, 2017

I can't understand background-foreground meaning

These should be self explanatory—it's the foreground color and the background-color—but if they're not, we should revisit. It's meant to be a way to signal to theme builders and customizers that you can flip the dark text on light background into a light text on dark background.

@gseregni
Copy link

Ah ok. So putting foreground and background in $theme-colors maps ends up creating counterintuitive rules likes bg-foreground/bg-background. $body-bg and $body-color are not enough for that ?

@misterdai
Copy link

@mdo don't know if it helps but the plugin it's complaining about, in the tests, is present in the package.json file but not build/npm-shrinkwrap.json.

@mdo
Copy link
Member Author

mdo commented Jun 28, 2017

Still more to do:

Just pushed updates for tables and list groups. Still need a bit more refinement on these colors.

@mdo
Copy link
Member Author

mdo commented Jun 30, 2017

Removed the old $gray- variables in latest commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.