ENH: Simplify theme variable code and support Sphinx Design #746

drammock · 2022-06-21T19:46:10Z

closes #733

@12rambau note that even though these override rules ought to be "more specific" because they're nested inside html data themes, I still needed to add !important to get them to actually override the sphinx-design rules.

LMK if anything looks weird / anything got missed.

choldgraf

This looks good in general! I have a quick comments in there about how to structure the implementation etc, but the end-result looks very nice!

choldgraf · 2022-06-22T08:34:38Z

src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss

+
+html[data-theme="light"],
+html[data-theme="dark"] {
+  @each $name in $sd-semantic-color-names {


the SCSS magic is strong with you ✨

Can you add a link to the SCSS docs that explain how @each works? I think many developers won't be familiar with it so it will be helpful to have breadcrumbs to help them learn quickly if they hit this code

Can you add a link to the SCSS docs that explain how @each works?

done

choldgraf · 2022-06-22T08:35:40Z

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

+    --pst-color-#{$name}-text: #{text-color($value)};
+    --pst-color-#{$name}-highlight: #{mix(#000, $value, 15%)};
+  }
+  /* ↓↓↓ these are not sphinx-design overrides ↓↓↓ */


Hmm, I am worried about intermixing "special casing" CSS with "default CSS" in one place, I feel like this could be confusing given that Sphinx Design is not bundled with the theme.

Is there a way that we can cleanly separate the two so that one section is entirely "just this theme" and another section is clearly "sphinx design", or even better if we can put the sphinx design stuff just in the extensions/ folder?

I am worried about intermixing "special casing" CSS with "default CSS"

I tried and failed to keep them separate yesterday. Today I did better; this is now fixed.

choldgraf · 2022-06-22T08:38:25Z

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

-  --pst-color-text-base: rgb(201, 209, 217);
-  --pst-color-text-muted: rgb(192, 192, 192);
-  --pst-color-border: rgb(192, 192, 192);
+  $pst-semantic-colors: (


Could we somehow make this a dictionary-like object at the top of the file, with [light] and [dark] keys? Then we can access the colors via those keys in our SCSS logic below? Might make it easier to keep everything in one place.

Could we somehow make this a dictionary-like object at the top of the file, with [light] and [dark] keys?

done

src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss

choldgraf · 2022-06-22T08:45:45Z

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

-  * main colors
+  * main colors. the $pst-semantic-colors follow the naming convention in
+  * sphinx-design, to allow for color overrides in
+  * ../extensions/_sphinx_design.scss. NOTE: some of these colors (most notably


See my other point about intermixing sphinx-design and theme-specific colors, but if we're going to do it this way, can we be remove the uncertainty here? E.g., not "some of these colors", but instead list the specific colors this statement applies to. And remove the "probably", we should be precise and explicit because others without context will have a hard time reasoning what is a sphinx-design thing and what's a theme thing, as well as the implications of using those variables in the theme in general.

Sorry for being a stickler on this, but we recently spent a lot of effort trying to reduce our surface area of variables that are exposed to devs and to users, so just trying to make sure we stick with those goals :-)

can we remove the uncertainty here?

This point is now moot; all colors needed for sphinx-design overrides are now only in extensions/_sphinx_design.scss

drammock

I think I've addressed all your comments @choldgraf, and the local nox -s docs-live looks good to me. See what you think!

drammock · 2022-06-22T15:58:08Z

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

-  --pst-color-text-base: rgb(201, 209, 217);
-  --pst-color-text-muted: rgb(192, 192, 192);
-  --pst-color-border: rgb(192, 192, 192);
+  $pst-semantic-colors: (


Could we somehow make this a dictionary-like object at the top of the file, with [light] and [dark] keys?

done

drammock · 2022-06-22T15:59:03Z

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

+    --pst-color-#{$name}-text: #{text-color($value)};
+    --pst-color-#{$name}-highlight: #{mix(#000, $value, 15%)};
+  }
+  /* ↓↓↓ these are not sphinx-design overrides ↓↓↓ */


I am worried about intermixing "special casing" CSS with "default CSS"

I tried and failed to keep them separate yesterday. Today I did better; this is now fixed.

drammock · 2022-06-22T16:00:07Z

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

-  * main colors
+  * main colors. the $pst-semantic-colors follow the naming convention in
+  * sphinx-design, to allow for color overrides in
+  * ../extensions/_sphinx_design.scss. NOTE: some of these colors (most notably


can we remove the uncertainty here?

This point is now moot; all colors needed for sphinx-design overrides are now only in extensions/_sphinx_design.scss

drammock · 2022-06-22T16:00:30Z

src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss

+
+html[data-theme="light"],
+html[data-theme="dark"] {
+  @each $name in $sd-semantic-color-names {


Can you add a link to the SCSS docs that explain how @each works?

done

…n.scss Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

drammock · 2022-06-22T16:42:21Z

The failing test is "no unexpected sphinx warnings", and it's complaining because sphinx/pygments can't lex the SCSS file. This is apparently a known bug

I pasted the content of _colors.scss into https://pygments.org/demo/?lexer=scss and indeed it is choking on the $ characters (but only in certain expressions)

drammock · 2022-06-22T16:43:24Z

One easy solution is to not embed the entire _color.scss file in the documentation, and instead simply point users to the file using the GitHub URL. @12rambau @choldgraf WDYT?

drammock · 2022-06-22T16:45:31Z

here are the relevant sections from the RTD build:

light:

dark:

choldgraf

I had no idea that SCSS could do all of this stuff, it is basically like python! :-)

The implementation looks good to me (a few quick comments in there). The tests seem to be failing because Sphinx doesn't know how to parse the SCSS, I think in the docs here:

https://github.com/pydata/pydata-sphinx-theme/blob/main/docs/user_guide/customizing.rst#color-variables

Those docs are actually probably out of date now anyway, because the color variables are structured differently than they used to be...can you think of a way to have the same effect with this new structure? Doesn't need to be the exact same, but should be useful at helping others understand the variable structure and how to change them.

Here's the rendered page:

https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/customizing.html#color-variables

src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_design.scss

src/pydata_sphinx_theme/assets/styles/variables/_color.scss

choldgraf · 2022-06-22T17:00:32Z

oops just saw your extra comments - I'd be +1 on:

Just pointing users to the file instead of embedding it
Trying to add whatever useful contextual information we can think of so others know what variables they can/cannot modify themselves
Add a comment about the SCSS bug and link to the issue you shared above, saying this is why we're not including the colors file

drammock · 2022-06-22T17:34:43Z

Just pointing users to the file instead of embedding it

done

Trying to add whatever useful contextual information we can think of so others know what variables they can/cannot modify themselves

I think this is pretty clear now because we point them to _color.scss and that has all and only the variables they might want to mess with. Everything else is hidden in extensions/_sphinx_design.scss

Add a comment about the SCSS bug and link to the issue you shared above, saying this is why we're not including the colors file

done.

choldgraf · 2022-06-22T18:04:28Z

Ahh I bet what's going on with sphinx design is that our variables are defined first, and later the sphinx design variables are defined, effectively overriding them.

Can we think of a quick way to make our variable definitions load after all the extensions, but before the user supplied custom CSS file? If that'd be complicated i agree we should just go with the current implementation

drammock · 2022-06-22T18:57:46Z

Ahh I bet what's going on with sphinx design is that our variables are defined first, and later the sphinx design variables are defined, effectively overriding them.

I'm not certain but I think it's the opposite: sphinx-design's style sheet is evaluated first, in a context in which our re-definition of their variables hasn't happened yet, and the value of those variables gets "frozen" into the rules. I think this SO answer backs that up, though the use case is a little different: https://stackoverflow.com/a/52015817

it's a really long answer, so I'll excerpt here two things that sum it up:

The 3 variables (--r, --g, --b) are evaluated inside :root so we have already substituted them with their values. [...] Considering this, we should always keep the evaluation at the lowest possible point in the DOM tree and especially after the variable changes (or at the same level)

If that's applicable here, I think there is actually no way to override CSS variables in file A with new values in file B and have that change affect rules defined in file A. You just have to rewrite those rules (or provide different rules with higher specificity).

I would love to be wrong about this, but I'm really bumping up against the limits of my understanding of the CSS cascade. Maybe someone else will recognize what I've done as an anti-pattern an fix it in a later PR :)

drammock · 2022-06-22T19:02:37Z

...aaand, I'm totally wrong. Pushing a fix that eliminates the rule-rewriting, and everything seems to still look correct. 🤷🏻

drammock · 2022-06-22T19:19:27Z

rendered docs: https://pydata-sphinx-theme--746.org.readthedocs.build/en/746/demo/kitchen-sink/web-components.html?highlight=web%20components#badges-and-buttons

CI failure looks like a fluke:

/opt/hostedtoolcache/Python/3.10.5/x64/bin/pip cache dir
/home/runner/.cache/pip
Error: getCacheEntry failed: connect ETIMEDOUT 13.107.43.16:443

can someone with permissions re-run that one job?

drammock · 2022-06-22T20:49:35Z

ok, all green except for the macOS job which was cancelled (?) so I think this one is ready!

12rambau

Thanks @drammock for this PR!

I didn't had the time to check it but that was nice reading your exchange. Appart from the few comments I made I'm very satisfied with this final version. it works as expected and ensure that the theme remains a nice neighbour.

@choldgraf I started to play with sass in the PR on sidebar display and I think there's a lot of refactoring to make in our files (but that will be for another PR)

docs/demo/kitchen-sink/web-components.rst

drammock · 2022-06-22T22:57:40Z

@12rambau I think I've addressed your comments. It helped catch something I had missed, which was that previously downstream users overriding --pst-color-whatever would not have affected badges and buttons! Now it will, though at the cost of creating more CSS variables in our --pst-color-* namespace. However, those extra variables are only appearing in the extensions/_sphinx_design.scss file, so theme contributors will probably not even see them in most cases. Hopefully that's good enough?

Here's the doc section, still looking good:

https://pydata-sphinx-theme--746.org.readthedocs.build/en/746/demo/kitchen-sink/web-components.html#badges-and-buttons

and CIs are all green.

12rambau

LGTM

choldgraf

This looks good to me as well, I really like the conciseness of the SCSS rules, many thanks for stepping through all this work @drammock 🚀

choldgraf reviewed Jun 22, 2022

View reviewed changes

drammock commented Jun 22, 2022

View reviewed changes

drammock and others added 10 commits June 22, 2022 11:02

add component examples to docs (verify the overrides work)

e59d326

typo: grammar

532337b

remove SD overrides from custom.css

efeedd8

overhaul color variable definition to allow overrides

d5fa199

add color rule overrides

dbb6fe5

fix shadow overrides so they actually work

fb0ac2a

Update src/pydata_sphinx_theme/assets/styles/extensions/_sphinx_desig…

cdff2b7

…n.scss Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

improve code comments

25b62d0

move panel color override to extensions

8e01900

refactor color defs; strictly separate overrides into extensions file

47bb4eb

drammock force-pushed the sd-color-overrides branch from 9996d6e to 47bb4eb Compare June 22, 2022 16:03

choldgraf reviewed Jun 22, 2022

View reviewed changes

choldgraf changed the title ~~color overrides for sphinx-design~~ ENH: Color variables for Sphinx Design and unify theming infrastructure Jun 22, 2022

drammock added 3 commits June 22, 2022 12:18

link back to what we borrow from sphinx-design

2bba561

update customizing section of user guide

a2cefbf

tweak code comment

c5e49ff

simplify

3dec9cd

12rambau reviewed Jun 22, 2022

View reviewed changes

docs/demo/kitchen-sink/web-components.rst Show resolved Hide resolved

docs/demo/kitchen-sink/web-components.rst Outdated Show resolved Hide resolved

drammock added 4 commits June 22, 2022 17:28

use button-ref

4e3a41f

cross-ref from web components to color customization docs

85ccfe8

make what the docs now say actually be true

4183efa

make code comments accurate

332cfef

12rambau approved these changes Jun 23, 2022

View reviewed changes

choldgraf approved these changes Jun 23, 2022

View reviewed changes

choldgraf changed the title ~~ENH: Color variables for Sphinx Design and unify theming infrastructure~~ ENH: Simplify theme variable code and support Sphinx Design Jun 23, 2022

choldgraf merged commit 7829e4a into pydata:main Jun 23, 2022

drammock deleted the sd-color-overrides branch June 23, 2022 10:07

jarrodmillman added this to the 0.10 milestone Jul 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Simplify theme variable code and support Sphinx Design #746

ENH: Simplify theme variable code and support Sphinx Design #746

drammock commented Jun 21, 2022

choldgraf left a comment

choldgraf Jun 22, 2022

drammock Jun 22, 2022

choldgraf Jun 22, 2022

drammock Jun 22, 2022

choldgraf Jun 22, 2022

drammock Jun 22, 2022

choldgraf Jun 22, 2022

drammock Jun 22, 2022

drammock left a comment

drammock Jun 22, 2022

drammock Jun 22, 2022

drammock Jun 22, 2022

drammock Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

choldgraf left a comment

choldgraf commented Jun 22, 2022

drammock commented Jun 22, 2022

choldgraf commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

12rambau left a comment

drammock commented Jun 22, 2022

12rambau left a comment

choldgraf left a comment

ENH: Simplify theme variable code and support Sphinx Design #746

ENH: Simplify theme variable code and support Sphinx Design #746

Conversation

drammock commented Jun 21, 2022

choldgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drammock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

choldgraf left a comment

Choose a reason for hiding this comment

choldgraf commented Jun 22, 2022

drammock commented Jun 22, 2022

choldgraf commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

drammock commented Jun 22, 2022

12rambau left a comment

Choose a reason for hiding this comment

drammock commented Jun 22, 2022

12rambau left a comment

Choose a reason for hiding this comment

choldgraf left a comment

Choose a reason for hiding this comment