twbs / bootstrap-npm-starter

Starter template for new building with Bootstrap 4 in npm projects.
MIT License
1.23k stars 465 forks source link

Issue with variable overrides #43

Closed ahsan-sabir closed 3 years ago

ahsan-sabir commented 3 years ago

Overriding some variable default only affect that variable, and does not affect other variables using that variable.

For example here's a snippet from _variables.scss

$primary:       $blue !default;
.
.
.
$link-color:    theme-color("primary") !default;

Now if I override the variable $primary then it will not reflect in $link-color.

This will not work

@import "bootstrap/scss/functions"; // Required
@import "bootstrap/scss/variables"; // Required
@import "bootstrap/scss/mixins"; // Required

//
// Override Bootstrap here
//
$primary: red;

Direct override will work

@import "bootstrap/scss/functions"; // Required
@import "bootstrap/scss/variables"; // Required
@import "bootstrap/scss/mixins"; // Required

//
// Override Bootstrap here
//
$primary: red;
$link-color: red; // All other elements using $primary will still appear $blue
Screen Shot 2021-02-11 at 7 41 12 PM

Issue appears to be inherent in bootstrap, and not with this starter template.

mdo commented 3 years ago

theme-color() functions aren't using the variables, they're pulling from the $theme-color map. It does appear that Sass doesn't update a map's values if the default value of one of its items is updated. Wonder if this is a bug in our implementation in Bootstrap 4, or just a limitation of Sass. Either way, we changed this in v5 so that it's easier to modify things (no more theme-color() function usage), but for now, we might be a little stuck with it in v4.x.

Reproducing the problem here: https://www.sassmeister.com/gist/89886b1f1917dfd5307df1c315e8ef67

mdo commented 3 years ago

Experimenting more, it seems the solution is to remove the default primary key from the original Sass map, then add a new one via map-merge(). Sorry this is a pain in the ass, but maybe there's another solution.

Check this for now: https://www.sassmeister.com/gist/8e0efb7fc7c6d6d2d55df87afe4e30c2

ahsan-sabir commented 3 years ago

This isn't the case with just maps, this is happening with variable values as well

For example

$font-size-base:              1rem !default;
$input-btn-font-size:         $font-size-base !default;
$btn-font-size:               $input-btn-font-size !default;
$input-font-size:             $input-btn-font-size !default;

Changes in $font-size-base is not reflecting in other variables (Tested on v5 branch)

mdo commented 3 years ago

Drop the !default from those replacement values and they should work.

ahsan-sabir commented 3 years ago

Sorry that was just the unchanged snippet from _variables.scss to show that no map is being used for $font-size-base

Here's the updated starter.scss

@import "bootstrap/scss/functions"; // Required
@import "bootstrap/scss/variables"; // Required
@import "bootstrap/scss/mixins"; // Required

//
// Override Bootstrap here
//
$font-size-base: 2rem;
// $btn-font-size: 2rem; // Will not work without directly overriding it
Screen Shot 2021-02-12 at 1 38 59 AM

The button font size remains unaffected

ahsan-sabir commented 3 years ago

It turns out the required imports should be after the overrides

//
// Override Bootstrap here
//
$font-size-base: 2rem;

// Required
@import "bootstrap/scss/functions"; // Required
@import "bootstrap/scss/variables"; // Required
@import "bootstrap/scss/mixins"; // Required

So I guess it's a matter of documentation. starter.scss and bootstrap docs like https://getbootstrap.com/docs/5.0/customize/sass/#importing needs to be updated.

Edit: There are some breaking changes when required imports are moved.

StudioKonKon commented 3 years ago

Your right, the required imports makes more sense to be after the overrides. I haven't had any issues with v5 doing this. Adding or removing maps do need to come after though since Bootstrap doesn't use map-merge after checking if a map already exists with !default.

The docs do seem to be a bit confusing. I initially thought the point of !default is to define a variable with a set value if it doesn't already exist or equal null. So don't know why any other reason our custom variables need to be included after Bootstrap's unless you want to reference Bootstrap's variables in your own but then other variables don't get updated.

Sass doesn't update variables if the one it's linking/referencing to changes as they are not actually referenced. It's more like they are cloned or copied values rather than shared values. For example:

// defaults
$a: 111 !default;
$b: $a !default;

// Variable Override
$a: 999;

.mycss-A { width: $a; } // 999
.mycss-B { width: $b; } // 111

https://www.sassmeister.com/gist/7113d09a69b84960531b2036e95cdc02

As seen here, $b was not updated when $a changed and still returns 111 even though $a had been overridden.

In Bootstrap's case, the problem starts in @import "bootstrap/scss/variables" this is happening:

// @import "bootstrap/scss/variables"
$blue:  #0d6efd !default;
$primary: $blue !default; // => #0d6efd
$link-color: $primary !default; // => #0d6efd
$font-size-base: 1rem !default;
$input-btn-font-size: $font-size-base !default; // => 1rem
$btn-font-size: $input-btn-font-size !default; // => 1rem
// Custom variables
$primary: red;
$font-size-base: 2rem;
// @import "bootstrap/scss/buttons"
.btn {
.
.
  @include button-size($btn-padding-y, $btn-padding-x, $btn-font-size, $btn-border-radius);
}
// @import "bootstrap/scss/reboot"
a {
  color: $link-color;
}
// CSS Output
.btn {
.
.
   font-size: 1rem;
}

a {
  color: #0d6efd;
}

Here, $btn-font-size was not updated because it was already set to 1rem before $font-size-base was overridden. The same goes for $link-color.

If you want to include custom variables after Bootstrap's variables like the docs state, then you'll need to update every other variable that uses $font-size-base so the changes are reflected and remember to remove all !default tags but doesn't that make the code more messier and less manageable?

$font-size-base: 2rem; // set new font size

// update every other variable that uses $font-size-base
$font-size-sm:  $font-size-base * .875;
$font-size-lg: $font-size-base * 1.25;
.
.
.
$input-btn-font-size: $font-size-base;
$btn-font-size: $input-btn-font-size;
.
.
.

The other option is to include your custom variables after the functions so you can use them in your code but before Bootstrap's defaults as these defaults will only apply if they haven't already been set in your custom variables. For example:

@import "bootstrap/scss/functions"; // Required

//
// Override Bootstrap defaults here
//
$font-size-base: 2rem;
$primary: red;

@import "bootstrap/scss/variables"; // Required ($primary and $font-size-base will replace the default)
@import "bootstrap/scss/mixins"; // Required

//
// Make any changes to Bootstrap's maps here (add / remove)
//
$custom-color: "#fe669e";
$theme-colors: map-merge(
  (
    "custom": $custom-color
  ),
  $theme-colors
);

!defaults in custom variables will still replace Bootstraps defaults and that makes it easier for others to make child themes without altering the main theme built on Bootstrap.

There doesn't seem to be an issue with Bootstrap, just the way Sass reads variables rather than across the entire project before compiling the CSS. Something I think LESS does? There doesn't seem to be a better way around it.

ahsan-sabir commented 3 years ago

There's another issue with required imports after variable overrides in v5 (works fine on v4).

btn-outline-* shows black text color on hover when it should be white.

starter.scss

// Configuration
@import "bootstrap/scss/functions";

//
// Override Bootstrap here
//
$primary: red;

@import "bootstrap/scss/variables";
@import "bootstrap/scss/mixins";
@import "bootstrap/scss/utilities";

index.html

<button type="button" class="btn btn-outline-primary" data-bs-toggle="modal" data-bs-target="#exampleModal">Click me!</button>

Without Hover With Hover
Screen Shot 2021-02-13 at 2 05 09 AM Screen Shot 2021-02-13 at 2 05 20 AM

I haven't tested much with required imports after variable overrides, so more issues are expected.

StudioKonKon commented 3 years ago

That issue with btn-outline-* showing red with black text seems to be an issue with the color-contrast($color) function not the required import.

Testing this code shows the issue:

$red: red;

.bg-red {
  background-color: $red;
  color: color-contrast($red); // => #000
}

The color-contrast function is for some reason returning black. You could lower the contrast ratio but I know there'll be some people who would be against that probably. Setting $min-contrast-ratio to 3.9 or lower instead of the default 4.5 makes the text white. Black on red is certainly not readable.

In v5.0.0-alpha2 the color contrast ratio was changed from 3:1 to 4.5:1 https://getbootstrap.com/docs/5.0/migration/#colors

// Configuration
@import "bootstrap/scss/functions";

//
// Override Bootstrap here
//
$primary: red;
$min-contrast-ratio: 3.9;

@import "bootstrap/scss/variables";
@import "bootstrap/scss/mixins";
@import "bootstrap/scss/utilities";

Issue with color-contrast at "bootstrap/scss/mixins/_buttons.scss" Line 77 $color-hover: color-contrast($color)

Seems to work with other colours like blue but not red. I don't know if this is something that should be reported as an issue on the main Bootstrap repository?

ahsan-sabir commented 3 years ago

Thank you for the explanation. Yes I don't think this needs to be reported, as overriding color-contrast or updating custom theme colors to reach a new minimum contrast is simple. But the documentation needs to be updated.

mdo commented 3 years ago

Damn, this is an annoying issue for sure. Let me summarize before we make any docs changes...

To override reassigned default variables (which we use like everywhere), we need to put the custom variables before our imports. But, if you want to use our own variables in your overriding, like $primary: $purple-500, they have to come after our imports, and they come with the problem of not updating other reassigned variables.

Is there a solution that works for both? Wondering if I've been missing something with Sass variables all this time.

markus-ksg commented 3 years ago

I think the only solution would be to split the default variable settings into dependency levels and then let clients insert their overrides at the right place.

One way to do this would be to have separate scss files / imports:

// overrides for base variables ($white, $blue, ..., $spacer, ...) 
@import "variables";
// overrides for dep-1 variables ($primary, $secondary, ..., $blue-100, ..., $spacers, ...)
@import "variables-dep-1";
// overrides for dep-2 variables ($link-color, ...)
@import "variables-dep-2";
// overrides for dep-3 variables ($link-hover-color, ...)
@import "variables-dep-3";

With the !default flag, there is some leeway here:\ An assignment like $red: #c00; works before or after $red: #dc3545 !default; , but it must come before any reference to $red.

Another solution would be to provide hook functions in _variables.scss that allow clients to insert their tweaks in the right place:

...
$cyan:    #0dcaf0 !default;
// scss-docs-end color-variables

@use "sass:meta";
@if meta.function-exists("variables-hook-1") {
  $dummy: meta.call(meta.get-function("variables-hook-1"));
}

Clients would then define their overrides like this:

@function variables-hook-1() {
  $red:     #c00 !global;
  $primary:       $red !global;
  $danger:        $orange !global;
  @return null;
}
@import "bootstrap";

Unfortunately, this spews a compile-time message, and I have not found a way to silence it:

DEPRECATION WARNING: As of Dart Sass 2.0.0, !global assignments won't be able to
declare new variables.

I'm currently working on a project where a modified $red is the primary color, and $orange the danger color. With v5.0.0-beta2, I had to duplicate the assignment of $orange: #fd7e14;:

$red:     #c00;
$orange:  #fd7e14; // same as default, but need to pre-define here, so we can use it to override $danger below
$primary:       $red;
$danger:        $orange;
@import "bootstrap";
StudioKonKon commented 3 years ago

Unfortunately, using @use At-Rules is only supported by compilers using the latest Dart Sass. It will not work with other compilers. Introducing @use in Bootstrap would break a lot of peoples code.

Assuming you are able to use @use At-Rules in your application, another solution would be:

// _bootstrap-required.scss
@import "../node_modules/bootstrap/scss/functions";
@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/mixins";
@import "../node_modules/bootstrap/scss/utilities";
// our-styles.scss

// "@use" bootstraps required imports
@use "bootstrap-required" as bootstrap;

// Override Bootstrap here
$primary: bootstrap.$purple-500;
$warning: bootstrap.shift-color($primary, 40%); // Example only showing use of functions

// Required
@import "bootstrap-required";

// Optional
@import "../node_modules/bootstrap/scss/buttons";
@import "../node_modules/bootstrap/scss/alert";
@import "../node_modules/bootstrap/scss/tables";
.
.
.

Here, we @use bootstrap with the namespace bootstrap as we want to keep them separate and don't want any conflicts. We also want to use bootstraps own variables, functions and mixins in our own too. Example: bootstrap.$purple-500 or bootstrap.shift-color()

We then override the defaults with our own before importing bootstraps required imports.

However, like I mentioned. Not all compilers support the @use At-Rules. Only Dart Sass currently supports it. Applications such as Koala and Hugo, ones I have tested and regularly use, don't work. Others include, Visual Studio or any that use LibSass and RubySass don't work either.

Node.js does support this, however.

Due to its lack of compatibility, it's not a solution for everyone, even if it looks cleaner. https://sass-lang.com/documentation/at-rules/use


Edited 2021/03/14

As far as Bootstrap's file and import structure goes, I don't think it should be changed as it's unnecessary and changing it so it will support many different specific compilers would lead to far more issues than there already is to maintain.

Overriding variables before Bootstrap's required imports is fine but the main issue is "how do we use Bootstrap's variables" since they haven't been imported yet? As Mark already stated:

To override reassigned default variables (which we use like everywhere), we need to put the custom variables before our imports. But, if you want to use our own variables in your overriding, like $primary: $purple-500, they have to come after our imports, and they come with the problem of not updating other reassigned variables. https://github.com/twbs/bootstrap-npm-starter/issues/43#issuecomment-780965131

Unfortunately, this is just how Sass/Scss works and nothing Bootstrap can do about it. They could restrict their package to using the latest version of Dart that supports @use and turn Bootstrap into a module which would make overriding variables a lot easier and/or wait for LibSass to update their code to support the @use At-Rule which won't be anytime yet. RubySass is no longer maintained so there's no worry there.

Outlook for LibSass 4.0

1 May 2020 — . . . beta released by end of this year, but it will not yet contain the new @use import feature from sass though. Maybe somebody wants to sponsor this feature to be added to LibSass sooner than later? https://github.com/sass/libsass/releases/tag/3.6.4

Due to this, I think it's best to either switch to Dart if you can or include your own way of overriding Bootstrap's variables and wait for a LibSass update where @use and @forward. can be implemented.

Alternative for LibSass

You could make use of scoped variables to do something similar to @use. All we want is to use Bootstrap's defaults before they are imported so we can override them without variable conflicts.

// _bootstrap-required.scss

// This doesn't output any CSS as we don't want any just yet.
@import "../node_modules/bootstrap/scss/functions";
@import "../node_modules/bootstrap/scss/variables";
@import "../node_modules/bootstrap/scss/mixins";
@import "../node_modules/bootstrap/scss/utilities";
// our-styles.scss

// Scope bootstraps required imports. These need to be defined globally first:
$primary: null;
$warning: null;

%scope {
  // import Bootstrap locally so to avoid variable conflict issues when importing globally.
  @import "bootstrap-required";

  // Override Bootstrap using Bootstrap's defaults using !global
  $primary: $purple-500 !global;
  $warning: shift-color($primary, 40%) !global;
}

// Override Bootstrap here where Bootstrap's defaults are not accessible nor needed.
$danger: #ea4335;

// Required
@import "bootstrap-required";

// Optional
@import "../node_modules/bootstrap/scss/buttons";
@import "../node_modules/bootstrap/scss/alert";
@import "../node_modules/bootstrap/scss/tables";
.
.
.

What do I mean, conflicting variables? Basically, if Bootstrap is imported globally twice. Many variables don't get overridden. E.g: If we change this: $font-size-base: 2rem;, the output CSS for $font-size-sm still shows .875rem. That's because the old !default overrode the new !default preventing $font-size-sm from updating.

Also, the reason for using null in $primary: null; is because a variable must be defined globally before overriding in the local %scope but we want bootstrap to use it's own default variables without overriding them unless you want to override the base values first and use the new generated non-base variables locally.

%placeholder

On another note, Bootstrap could make use of the %placeholder to make extending selectors more easier and reduce file size by avoiding duplicate classes and styles in custom components. (E.g: adding a new style to .btn or .card not present in Bootstrap without creating a separate selector for it, especially for long selectors like .nav-tabs .nav-link:hover, .nav-tabs .nav-link:focus).

WinterSilence commented 3 years ago

@StudioKonKon @use work only in dart ((

problem in current architecture:

Right way:

  1. Separate packages

Required global package with common mixins, variables like as $enable-* and @import optional packages.

// re-declared variables

@import "global/variables";
@import "global/mixins";

$packages: typography, ... !default;
@each $package from $packages {
     @import "packages/#{$package}/package";
}

Packages contains optional components/parts, can use global variables/mixins, contain config and documentation,

Package "Typography":

  1. "step" variables with increment values:
    
    // global/_variables.scss
    $step-size: .25 !default;
    $step-size-sm: .125 !default;
    $step-size-lg: .5 !default;

$font-size: 1rem !default; $font-size-sm: $font-size - $step-size !default; $font-size-lg: $font-size + $step-size !default;

// typography/_variables.scss $typography-font-size-base: $font-size + 1 !default; $typography-font-size-step: $step-size-sm !default;

// typography/_headers.scss @for $i from 1 through 6 { h#{$i}, .h#{$i} { font-size: $typography-font-size-base + ($typography-font-size-step * $i); } }

lorvent commented 3 years ago

Oh! my goodness!

I have spent hours today tinkering with customizing variables like blue primary and everytime i come across some or other error.

i tried following bootstrap theming guide option A and option B thinking that is the correct way, none of them.

finally when i do something like

$blue : green;
$colors: ( "blue" : $blue);
$theme-colors: (
  "custom-color": #900
);
@import 'node_modules/bootstrap/scss/bootstrap';

it works but i am keep going to back to recommended options only to get compile errors...finally this thread made me realize there is a problem with documentation itself 🤦

dgobnto commented 3 years ago

After reading this issue I am still not sure if there is / which is the right solution for overriding variable defaults in Bootstrap 5.0… I've noticed that in the docs (https://getbootstrap.com/docs/5.0/customize/sass/#variable-defaults) the example doesn't reflect what is written in the paragraph above it. Which one should I follow? Thank you!

mdo commented 3 years ago

@dgobnto v5's docs will be updated with the next release, which is coming in the next week or two.

dgobnto commented 3 years ago

@mdo And in the meanwhile, should I follow the code example (functions > overrides > variables > mixins) or the description (functions > variables > mixins > overrides)?

sviluppatoreLaRegione commented 3 years ago

still not yet updated docs? Which one we should follow, starter kit or docs? docs however said two different things.

mdo commented 3 years ago

v5 docs were updated when v5 stable shipped: https://getbootstrap.com/docs/5.0/customize/sass/#variable-defaults. v4.x may need more updates but we've prioritized v5 patches.

dgobnto commented 3 years ago

I'm sorry if it's just me being dumb but from what I can see, the docs remain confusing. Although the text says

Variable overrides must come after our functions, variables, and mixins are imported

the example code just bellow follows another order, namely: functions > variable overrides > variables > mixins.

mdo commented 3 years ago

Oh no, you're not being dumb, I can see the text is wrong, but the code snippet was updated. We updated the snippets, but missed the text in the docs: https://github.com/twbs/bootstrap/commit/6ed439ff7326f3912d9cbb0db9c5ec9e29dc9381#diff-c7298b94458b9d5c93aee66865a7492e32457939be4317ea16958b47a43b732e.

The fix is coming in v5.0.2, see #34179.

sviluppatoreLaRegione commented 3 years ago

@mdo Thanks for fix, I mean that part was confusing.

Anyway why this starter kit set custom variables before functions? https://github.com/twbs/bootstrap-npm-starter/blob/main/scss/starter.scss#L28-L35