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

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

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

chalin
Copy link
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
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.

$link-color: darken($blue, 15%) !default;
$link-color: adjust-color($blue, $lightness: -15%) !default;
Copy link
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
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.

None yet

2 participants