-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Make it easier to customize Sass variables #4506
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2d3d369
to
c6bc0fc
Compare
c6bc0fc
to
abd1ad4
Compare
abd1ad4
to
c0d92d2
Compare
taitus
approved these changes
Jun 29, 2021
c7cdae8
to
aa11c06
Compare
It was defined twice in the same file. Since it's also defined by Foundation, I'm keeping the one in the "Foundation overrides" section.
Foundation does not have a variable named `$font-family-serif`.
We don't use accordions anywhere; these settings probably come from an earlier Foundation version and we forgot to remove them.
We were loading all Foundation variables and then overriding them. This is problematic because we were overriding variables after using them. For instance, we had this code in `_settings.scss`: ``` $black: #0a0a0a; $white: #fefefe; $body-background: $white; $body-font-color: $black; ``` That meant we were setting `$body-background` to `#fefefe`, and `$body-font-color` to `#0a0a0a`. Even if we changed the values of `$black` and `$white` later, `$body-background` and `$body-font-color` were not affected by this change. So now we're overriding `$black` and `$white` before setting `$body-background` and `$body-font-color`. In order to keep our custom values instead of overriding them, we're using the `!default` flag in the `_settings.scss` file. This way of defining variables in the `_settings.scss` file with the `!default` flag is recommended in the documentation regarding the settings file [1]. There's also a disadvantage to this approach. Now that we're loading the `_consul_settings.scss` file first, we can no longer use Foundation variables inside the `_consul_settings.scss` file unless we define them in this file as well. [1] https://get.foundation/sites/docs/sass.html#the-settings-file
We were overriding these values because the `$body-font-family` and `$global-radius` variables were defined after the values in the `_settings.scss` file had been computed. Now these values are defined before the values in the `_settings.scss` are computed, so we don't need to override the values which depend on them anymore.
Now that we load this file before loading the `_settings.scss` file, we can safely use the `!default` flag. This makes it easier to override these variables by adding a new file and loading it before `_consul_settings.scss` is loaded. For instance, we've got this code related to the `$brand` variable: ``` $brand: #004a83 !default; $brand-secondary: darken($brand, 10%) !default; $dark: $brand-secondary !default; $link: $brand !default; $debates: $brand !default; $tooltip-background-color: $brand !default; ``` If we override `$brand` in `_custom_settings.scss`, variables like `$link` won't be affected by this change. In order to do so, we'd need to load `_custom_settings.scss` before loading `_consul_settings.scss`. So why aren't we loading `_custom_settings.scss` first, just like we load `_consul_settings.scss` before loading `_settings.scss`? Mainly, compatibility reasons. Some people might have this code in their `_custom_settings.scss` file: ``` $dark: darken($brand, 30%); ``` If we load this file before loading `_consul_settings.scss`, we'll get an error because `$brand` isn't defined at this point. So we're introducing a new file where variables can be overriden before they are defined while keeping the option to override them after they are defined. We're updating the comments in these files to define the new behavior, and removing the links (which point to places which don't exist since commit 392d633) in order to make it easier to read the comments.
So we make sure the rules we add will take precedence over vendor rules.
aa11c06
to
38436b1
Compare
Senen
approved these changes
Jun 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Background
Currently our
_consul_settings.scss
file has the following code:In order to override these values, overriding the
$brand
variable in_custom_settings.scss
is not enough; all variables depending on it need to be overridden as well.Objectives
Notes
The way we now load Foundation variables has a minor drawback: now we can't use any Foundation variables in the
_consul_settings.scss
file unless we define them there first. The same would happen if we loaded_custom_settings.scss
first; we aren't doing so for compatibility reasons. Some people might have this code in their_custom_settings.scss
file:If we load this file before loading
_consul_settings.scss
, we'll get an error because$brand
isn't defined at this point.