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.

Sign up for GitHub

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

Jump to bottom

Make it easier to customize Sass variables #4506

Merged
merged 8 commits into from
Jun 29, 2021
Merged

Make it easier to customize Sass variables #4506

merged 8 commits into from
Jun 29, 2021

Conversation

javierm
Copy link
Member

@javierm javierm commented May 9, 2021 •
edited
Loading

References

Background

Currently our _consul_settings.scss file has the following code:

$brand:             #004a83;
$brand-secondary:   darken($brand, 10%);
$dark:              $brand-secondary;
$link:              $brand;
$debates:           $brand;

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

  • Make it easier for other CONSUL installations to override colors, font sizes, ...
  • Simplify the code we use to override Foundation settings
  • Fix a bug which made us override Foundation settings only in some places

Notes

⚠️ We need to update the documentation explaining how to customize CONSUL.

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:

$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.

@javierm javierm self-assigned this May 9, 2021
@javierm javierm added this to Reviewing in Consul Democracy via automation May 9, 2021
@taitus taitus self-assigned this Jun 29, 2021
Consul Democracy automation moved this from Reviewing to Testing Jun 29, 2021
@javierm javierm force-pushed the foundation_settings branch 2 times, most recently from c7cdae8 to aa11c06 Compare June 29, 2021 13:08
@javierm
Remove redundant variable
59cbfd2 
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.
@javierm
Move CONSUL variable to CONSUL variables section
4474ec4 
Foundation does not have a variable named `$font-family-serif`.
@javierm
Remove unused Sass variables
fc0db19 
We don't use accordions anywhere; these settings probably come from an
earlier Foundation version and we forgot to remove them.
@javierm
Override Foundation variables everywhere
1047914 
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
@javierm
Remove no longer necessary overrides
bb164f8 
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.
@javierm
Overwrite close button color using Sass variables
211e400
@javierm
Make it easier to override CONSUL Sass variables
52bc16b 
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.
@javierm
Load vendor CSS before application CSS
38436b1 
So we make sure the rules we add will take precedence over vendor rules.
@javierm javierm merged commit a81aac6 into master Jun 29, 2021
Consul Democracy automation moved this from Testing to Release 1.4.0 Jun 29, 2021
@javierm javierm deleted the foundation_settings branch June 29, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Senen Senen Senen approved these changes

@taitus taitus taitus approved these changes

Assignees

@Senen Senen

@taitus taitus

@javierm javierm

Labels
Customization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@javierm @Senen @taitus