Skip to content

[BSv5] Fix SCSS functions import issue, and replace darken()#1388

Merged
chalin merged 1 commit intogoogle:mainfrom
chalin:chalin-im-bsv5-darken-2023-02-01
Feb 1, 2023
Merged

[BSv5] Fix SCSS functions import issue, and replace darken()#1388
chalin merged 1 commit intogoogle:mainfrom
chalin:chalin-im-bsv5-darken-2023-02-01

Conversation

@chalin
Copy link
Copy Markdown
Collaborator

@chalin chalin commented Feb 1, 2023

⚠️ See below for an IMPORTANT DESIGN NOTE!

This PR:

Followup:


⚠️ Design note regarding functions import fix

Is this fix really necessary?

Without the import fix included in this PR, use of Bootstrap functions can result in bogus errors like the following:

argument $color of <some-bs-function>(...) must be a color

when in fact the argument is a color.

Learn more

For details concerning the need to import functions.scss first, see the Bootstrap migration guide under New _maps.scss

Design choice: double import vs. inlining many imports

In this PR, I'm proposing to @import "functions" before our variable overrides, and then import the full Bootstrap suite of SCSS. This will result in the "functions" file being loaded twice, but according to the Sass @import documentation:

If the same stylesheet is imported more than once, it will be evaluated again each time. If it just defines functions and mixins, this usually isn’t a big deal, but if it contains style rules they’ll be compiled to CSS more than once.

The _functions.scss file doesn't contain any style rules, so we should be ok. The alternative would be to inline all of the 40+ imports in https://github.com/twbs/bootstrap/blob/main/scss/bootstrap.scss, which would be a significant maintenance overhead, one that I think that we can avoid (until proven otherwise).

@chalin chalin requested review from LisaFC and geriom February 1, 2023 14:36
@@ -1,3 +1,5 @@
@import "../vendor/bootstrap/scss/functions";
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See the opening comment of this PR for a design rationale.

$td-box-colors: $dark, $primary, $secondary, $info, $gray-600, $success, $warning, $dark, $danger, $primary, $secondary, $info !default;

$link-color: darken($blue, 15%) !default;
$link-color: adjust-color($blue, $lightness: -15%) !default;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

While I followed BS's upgrade path and generally replaced darken() by shade-color(), in this case the resulting color didn't look as good, so I opted for a call to adjust-color() --matching the previous color exactly.

@chalin chalin mentioned this pull request Feb 1, 2023
50 tasks
@LisaFC
Copy link
Copy Markdown
Collaborator

LisaFC commented Feb 1, 2023

Makes sense to me! I don't see any obvious (or easy to maintain) alternative to this approach.

@chalin chalin merged commit 6012797 into google:main Feb 1, 2023
@chalin chalin deleted the chalin-im-bsv5-darken-2023-02-01 branch February 1, 2023 16:53
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.

2 participants