Integrates custom scss & callouts

[Ethan0429]

Tested locally with my own fork of just-the-docs, and it fixes the issue from #1011. As suggested by @pdmosses.

[pdmosses]

@Ethan0429 thanks for this!

I'm not familiar with the details of SCSS, but I think it's unlikely that importing the same file twice (instead of once) will break any sites.

[pdmosses]

@mattxwang I've also checked that just-the-docs-tests builds and displays a pink callout when using:

remote_theme: Ethan0429/just-the-docs@patch-1

[mattxwang]

Going to override @pdmosses' review here; I don't think we can merge this.

I'm not familiar with the details of SCSS, but I think it's unlikely that importing the same file twice (instead of once) will break any sites.

So, this is not true: if you import any SASS file twice, you are:

1. re-executing all the logic within the SASS file
2. re-emitting all CSS rules

The former could cause breaking behaviour (ex adding to a map twice), and the latter bloats code.

(you can test this by adding any CSS rule to custom.scss in the test branch)

I still agree that the issue is a problem, however I don't think that this current PR solves the problem. Bigger picture, my opinion is that this is a problem with the "overloading" of custom.scss. There are two distinct types of use-cases:

1. SASS variable declarations that we want to be accessible to our underlying generator code. This is a forward declaration; we want this to be added as soon as possible in the SASS cascade, but after our default variables.
2. Custom CSS rules that override existing generated code. We want this to be the very last thing in the cascade.

Ideally, what we should do (instead of this PR) is add an import that occurs right after our last variable declaration, and before our first emitted CSS - custom callout variables should go in there, instead of in custom.scss. This would also necessitate us updating our instructions for callouts.

Thoughts @pdmosses @Ethan0429 @deseo @clarisma (people involved in this PR and #1010)

[pdmosses]

I'm not familiar with the details of SCSS, but I think it's unlikely that importing the same file twice (instead of once) will break any sites.

So, this is not true: if you import any SASS file twice, you are:

1. re-executing all the logic within the SASS file
2. re-emitting all CSS rules

The former could cause breaking behaviour (ex adding to a map twice), and the latter bloats code.

1. From the docs for adding to a map or merging two maps, I don't see how adding the same key-value pair to a map twice could 'break behaviour'. Could you give an example?

2. However, I agree that added CC rules appear twice in assets/css/just-the-docs.default.css. That could double the size of the CSS for a site that wanted to completely customise the styling.3.

   I'm a bit surprised at the lack of optimisation in the resulting CSS. When the rules in custom.scss are independent of the SCSS used for callouts, I don't see how a document could be affected by removing the duplication. If CSS selectors can't distinguish between the two sets of rules, then the SCSS compiler could surely optimise and eliminate the bloat? But there may be trade-offs that I'm completely unaware of.

I still agree that the issue is a problem, however I don't think that this current PR solves the problem. Bigger picture, my opinion is that this is a problem with the "overloading" of custom.scss. There are two distinct types of use-cases:

1. SASS variable declarations that we want to be accessible to our underlying generator code. This is a forward declaration; we want this to be added as soon as possible in the SASS cascade, but after our default variables.
2. Custom CSS rules that override existing generated code. We want this to be the very last thing in the cascade.

Ideally, what we should do (instead of this PR) is add an import that occurs right after our last variable declaration, and before our first emitted CSS - custom callout variables should go in there, instead of in custom.scss. This would also necessitate us updating our instructions for callouts.

Thoughts @pdmosses @Ethan0429 @deseo @clarisma (people involved in this PR and #1010)

In the discussion of #1011, I suggested putting custom colors for callouts in the various color schemes, which are also imported before the modules. It's unclear to me whether such custom colors should be associated with particular color schemes or not. If not, then your suggestion seems to be the best.

[pdmosses]

In the discussion of #1011, I suggested putting custom colors for callouts in the various color schemes, which are also imported before the modules. It's unclear to me whether such custom colors should be associated with particular color schemes or not. If not, then your suggestion seems to be the best

If a user copies the built-in color schemes light.scss and dark.scss, and adds custom colors for callouts to them, the user's copies shadow any updates to those schemes made in future versions of the theme. For example, the theme might update code highlighting to be more like the colors on GitHub, and users should be able to benefit from that (without needing to copy and edit the files again).

Instead of copying files from the theme and editing them, users could define custom color schemes, say my-light and my-dark, that import the built-in schemes. They would then need to set color_scheme in _config.yml to avoid seeing the standard light scheme – and perhaps also dark_color_scheme when the theme gets toggling or auto-switching between light and dark.

This would be complementary to adding/setting variables that apply to all color schemes.

BTW, it seems a bit odd that custom color schemes are located in _sass/color_schemes/. Wouldn't _sass/custom/color_schemes/ be a more logical location for them?

[pdmosses]

Ideally, what we should do (instead of this PR) is add an import that occurs right after our last variable declaration, and before our first emitted CSS - custom callout variables should go in there, instead of in custom.scss.

@mattxwang Would you like me to submit a PR for importing a new custom file _sass/custom/variables.scss?

This would also necessitate us updating our instructions for callouts.

Right. If we're going to make this change, we'd better do it before the release of v0.4.0.

The docs for configuring callouts will need to warn against defining custom callout colors in _sass/custom/custom.scss.

I think custom callout colors that depend on the color scheme should be defined in _sass/color_schemes/*.scss; scheme-independent callout colors are to be defined in the new _sass/custom/variables.scss.

_includes/css/just-the-docs.scss.liquid is:

{% if site.logo %}
$logo: "{{ site.logo | relative_url }}";
{% endif %}
@import "./support/support";
@import "./color_schemes/light";
@import "./color_schemes/{{ include.color_scheme }}";
@import "./modules";
{% include css/callouts.scss.liquid color_scheme = include.color_scheme %}
{% include css/custom.scss.liquid %} 

./custom/variables should be imported after ./support/support.

When include.color_scheme is light, it's imported twice. To minimise the generated CSS, the second import needs to be conditional. I guess that change should be in a separate PR?

[mattxwang]

Thanks for your patience everyone, been a bit behind due to personal circumstances. Let me put the concise/actionable answers first.

@mattxwang Would you like me to submit a PR for importing a new custom file _sass/custom/variables.scss?

Personally leaning towards this, as

It's unclear to me whether such custom colors should be associated with particular color schemes or not. If not, then your suggestion seems to be the best.

My opinion is no, they should not; if they want to have this association, they could define it in a custom color scheme (like you've suggested in a later comment)

Right. If we're going to make this change, we'd better do it before the release of v0.4.0.
The docs for configuring callouts will need to warn against defining custom callout colors in _sass/custom/custom.scss.

Agreed and agreed. The nice thing is, if we do this all properly, none of this is a breaking change from v0.3.3: we're simply adding a new custom import and adding callouts. This is only breaking for prior prerelease users.

@Ethan0429, I know this has been a wall of text; does that make sense? Since you also did the initial investigation, I think it would be appropriate if you wanted to take on this PR and/or we made you a co-author of whatever we do end up merging.

---

Other responses:

From the docs for adding to a map or merging two maps, I don't see how adding the same key-value pair to a map twice could 'break behaviour'. Could you give an example?

Hm, you may be right - I made that comment previously since I had helped debug a coworker with this exact problem (they loaded a SASS file twice, and added to a map twice). However, that was a couple of years ago - I wonder if since then, the SASS spec has changed?

(can't remember the example now, but it may have been that the RHS of the set expression had some sort of mutable call)

No worries if I can't recall, we can strike this from the record if necessary.

However, I agree that added CC rules appear twice in assets/css/just-the-docs.default.css. That could double the size of the CSS for a site that wanted to completely customise the styling

I'm a bit surprised at the lack of optimisation in the resulting CSS. When the rules in custom.scss are independent of the SCSS used for callouts, I don't see how a document could be affected by removing the duplication. If CSS selectors can't distinguish between the two sets of rules, then the SCSS compiler could surely optimise and eliminate the bloat? But there may be trade-offs that I'm completely unaware of.

Certainly no expert, but in my work contributing to Stylelint I've noticed that all the no-duplicate rules aren't autofixable. My understanding is that generally, it's not possible to guarantee that removing either duplicate leads to the same rendered page / processed CSS, especially due to wonkiness in cascades.

BTW, it seems a bit odd that custom color schemes are located in _sass/color_schemes/. Wouldn't _sass/custom/color_schemes/ be a more logical location for them?

I have no strong opinion here! Happy to discuss (perhaps in an issue)

[pdmosses]

@mattxwang wrote:

Ideally, what we should do (instead of this PR) is add an import that occurs right after our last variable declaration, and before our first emitted CSS - custom callout variables should go in there, instead of in custom.scss. This would also necessitate us updating our instructions for callouts.

I've added a regression test for using custom colors in callout configurations. If this PR is changed to introduce a new custom file for variables, the test will also need updating.

[pdmosses]

@mattxwang wrote:

@Ethan0429, I know this has been a wall of text; does that make sense? Since you also did the initial investigation, I think it would be appropriate if you wanted to take on this PR and/or we made you a co-author of whatever we do end up merging.

@Ethan0429 Would you like to proceed with revising this PR, or submit a fresh one?

[Ethan0429]

@pdmosses Apologies for the late reply. I'll see to revising the PR ASAP. Thank you all for the support!

Review retracted following subsequent discussion

[simonebortolin]

(same comment made in issues)
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.

[mattxwang]

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.

Hm, @simonebortolin so I'm not sure if this is what I'm suggesting / the core problem.

• the proposed solution I outlined above (two separate files - one to define SASS variables up top, and one to define overrides at the bottom) still doesn't import two files twice - it splits up the logic so it's easier to deal with
• var() variables in CSS are runtime changes. They are useful in many ways, but are different than SASS (statically generated). Since they can be complicated for users to use and introduce more namespace collisions (SASS variables don't occupy the CSS namespace after compilation), I don't think it's a good solution for this issue.

The proposed solution is just code motion + documentation, so I anticipate it shouldn't be too complicated. @Ethan0429 what are your thoughts? There's no obligation at all to do this if you're not available; I can add it to my backlog (or re-outline this in an issue and have someone else pick it up) if you'd like. Contributions are always welcome!

[mattxwang]

Hi @Ethan0429, just wanted to check in on the state of this PR. Is there anything I can do to support you? Appreciate your initiative thus far!

[mattxwang]

Closed by #1135.