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

move theme-colors (vars+map) after color tints definitions #35592

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

pine3ree
Copy link
Contributor

In this way we can use color tints (other than grays) in our custom theme (e.g $primary: $indigo-600; ), w/o having to extend $theme-colors and redefine $theme-colors-rgblater. This could be done in the project variables file, but i believe most developers just include a full local copy of scss/_variables.scss before the the original default file and change the values the need (maybe also removing the !default flag) so that it's easier to track changes and custom values during upgrades.

@pine3ree pine3ree requested a review from a team as a code owner December 23, 2021 13:47
@ffoodd
Copy link
Member

ffoodd commented Jan 5, 2022

Not quite sure about this at all.The change looks inoffensive, but the scenario you describe is not what we currently recommend.

You're basically copying _variables.scss and change some values before compiling, is that right?

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Nevermind, I think it makes sense and is more readable this way. Thanks :)

@pine3ree
Copy link
Contributor Author

pine3ree commented Jan 6, 2022

Hello @ffoodd.
Thanks, this simple change (putting tint definitions before theme colors) only allows us to use predefined tints in our theme.

About the other matter (It was meant as a side note, not exactly 100% related :-) ):

You're basically copying _variables.scss and change some values before compiling, is that right?

Yes, but not only that. I usually do the following:

//  project's main style.scss file
@import "bootstrap/scss/_functions";

// Customize variables
@import "./_variables.scss"; // a copy of `bootstrap/scss/_variables` with the `!default` modifiers removed

@import "bootstrap/scss/_variables";  // this is the original bootstrap variables file, or any new version of it
@import "bootstrap/scss/_mixins";
//...
// other bootstrap imports

Since some bootstrap sass variables may depend on previous variables in the same file, it's easier for me to duplicate the original src distribution file and use it before itself. This helps me in 3 ways:

  1. The dependencies of variables are all there in my customized file, no need to go back to bootstrap vars if any dependency is missing. Let's suppose that a variable refer to another variable and I need to change it to refer to a third variable. If my custom variables file only includes the first dependency, I'll have to copy the third variable definitions (and its dependecies if any) to my file. Having a full local copy of the variables file let me skip all these steps.
  2. During upgrades, new variables are still there in the included new standard bootstrap variables file.
  3. It's easier to diff my customized variables file with any newer version and upgrade it accordingly.

I am not sure why, but I thought that most developers were doing it in the same way and for the same reasons :-)

kind regards

@ffoodd
Copy link
Member

ffoodd commented Jan 6, 2022

I see. Most people only override what they need (for clarity) but I get your point.

In this way we can use color tints other than grays in our custom theme, w/o having to extend it later (e.g `$primary: $indigo-600;` ). This could be done in the project variables file, but i believe most developers just include a full local copy of  `scss/_variables.scss` before the the original `default` file and change the values the need (maybe also removing the !default flag) so that it's easier to track changes and custom values during upgrades.
@mdo mdo merged commit d74870a into twbs:main Feb 25, 2022
@pine3ree pine3ree deleted the patch-5 branch February 25, 2022 23:10
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.

3 participants