Custom callout colors no longer work

[Ethan0429]

Describe the bug
This commit 53edd78 seems to have broken the ability to customize callout colors using custom defined colors from _sass/custom/custom.scss, as the build process fails (using Github Actions) with the following error:

github-pages 227 | Error:  Undefined variable: "$magenta-300". on line 9
      Remote Theme: Cleaning up /tmp/jekyll-remote-theme-20221024-7-d7owx1

To Reproduce
Steps to reproduce the behavior:

1. Create _sass/custom/custom.scss file
2. Add custom 000/300 color e.g. magenta-000, magenta-300
3. Add callouts customization in /_config.yaml e.g.

callouts:
  note:
    color: magenta

5. Build (using GitHub actions)

Expected behavior
As it has always worked before 53edd78, the build should complete, and callout colors will successfully overwrite using the custom colors defined.

Desktop

• OS: Linux
• Browser Firefox
• Version 106.0.1

[mattxwang]

Agh! At first glance, I think you're correct @Ethan0429. I didn't do a great job reviewing #1010 and clearly this is a consequence.

Let me do a bit of work when I'm more free to figure out a good solution to both this problem and the one that #1010 resolves. In the meantime, you should be able to pin the remote theme to a previous commit and/or a previous release. Sorry for the confusion!

cc: @pdmosses

[Ethan0429]

Agh! At first glance, I think you're correct @Ethan0429. I didn't do a great job reviewing #1010 and clearly this is a consequence.

Let me do a bit of work when I'm more free to figure out a good solution to both this problem and the one that #1010 resolves. In the meantime, you should be able to pin the remote theme to a previous commit and/or a previous release. Sorry for the confusion!

cc: @pdmosses

Awesome! It's not problem at all. Thank you for the quick reply!

[pdmosses]

@mattxwang I also reviewed this, and I should have checked that (1) we have a regression test for adding custom colors, and (2) the regression test site still builds. Mea culpa 🙄

In fact (1) is okay: the current callout tests define a pink callout, and the pink color shades are defined in custom.scss. But I neglected to check (2). And when I stopped and restarted my local build of just-the-docs-tests, the build failed in the same way as @Ethan0429 reported.

I think the root of the problem is that the theme docs make claims that are inconsistent. In Override and completely custom styles we have:

For styles that aren’t defined as variables, you may want to modify specific CSS classes. Additionally, you may want to add completely custom CSS specific to your content. To do this, put your styles in the file _sass/custom/custom.scss. This will allow for all overrides to be kept in a single file, and for any upstream changes to still be applied.

And in Callouts configuration:

to use a custom color, you need to define its 000 and 300 levels in your SCSS files. For example, to use pink, add the following to your _sass/custom/custom.scss file:

Custom colors were deliberately not used in the callout illustrations in the theme docs, so the deploy preview built okay.

[pdmosses]

I think the root of the problem is that the theme docs make claims that are inconsistent

More precisely: To allow configuration of callouts with custom colors, the implementation imported _sass/custom/custom.scss (via _includes/css/custom.scss.liquid) before _includes/css/callouts.scss.liquid. That was inconsistent with being able to override all CSS using custom.scss.

A solution might be to define custom callout colors in color schemes, which do get imported before _includes/css/callouts.scss.liquid. But assets/css includes CSS for both the light and dark color schemes (and for a scheme set by color_scheme in _config.yml), so custom callout colors would need to be added to all schemes, which is unattractive.

To work around this inconsistency, _sass/custom/custom.scss could be imported twice: both before and after _includes/css/callouts.scss.liquid. The relevant code in _includes/css/just-the-docs.scss.liquid would be:

{% include css/custom.scss.liquid %}
{% include css/callouts.scss.liquid color_scheme = include.color_scheme %}
{% include css/custom.scss.liquid %}

Then any color variables defined in custom.scss would be available for use in callouts, and CSS defined in custom.scss could override the CSS for callouts. (I suspect that it's possible to write (S)CSS which has a different effect when imported once or twice, but it seems unlikely that such code is used in practice.)

This workaround would correspond to PR #1010 but with only the addition of a line, omitting the deletion of a line.

For a future version of the theme, we might apply each callout configuration only to a specific color scheme, and insist on defining custom color variables in color schemes.

[pdmosses]

@mattxwang I wrote:

For a future version of the theme, we might apply each callout configuration only to a specific color scheme, and insist on defining custom color variables in color schemes.

The theme defines classes for button colors. Perhaps it should also define corresponding classes for callouts? Unfortunately, I have no expertise at all in SCSS, nor in color design Writing code like that in _sass/support/mixins/_buttons.scss is completely beyond me…

[simonebortolin]

(same comment made in PR)
I propose another much simpler and more acceptable solution, which most likely reduces the amount of code to write, since there is no 1-1 mapping between callout and system colours it would be convenient to use var(--) css instead of including a file twice. Since var(--) css are parsed by the browser this allows for easy customisation.