search

pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
616 stars 319 forks source link

Better documentation for how colors are configured #279

Closed choldgraf closed 2 years ago

choldgraf commented 4 years ago

I was trying to modify the default colors in sphinx-book-theme so that they reverted to the old defaults, but I could not figure out how to do so. In particular, it says you need to use CSS variables but not SASS variables, and I couldn't get this to work. For ref:

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

I found these things confusing:

  1. Theme developers may not know the difference between CSS and SASS variables
  2. It's unclear if / how it's possible to use both kinds of variables at the same time
  3. Documenting what :root means seems important
  4. The CSS variable syntax that this theme uses --default-color: 23, 54, 23; etc, seemed to throw an error with my SCSS compiler. Is this some kind of non-standard syntax?

Perhaps @hoetmaaiers could advise on best-practices here and we can make a docs PR?

EDIT: I think I got it working but I had to use a different syntax:

https://github.com/executablebooks/sphinx-book-theme/pull/246/files#diff-e5a2bc9e76b60200872fec420e32a890b2035706e458f23e9b901b4faad9ee6cR17

jorisvandenbossche commented 4 years ago

EDIT: I think I got it working but I had to use a different syntax:

Is it working with that or not?

choldgraf commented 4 years ago

I think that it's working but I am not 100% sure haha

jorisvandenbossche commented 4 years ago

But thanks for your questions! That's very useful to ensure this is clearly documented ...

Theme developers may not know the difference between CSS and SASS variables

We for sure need to clarify the current text, but in general I would say: if you are not using SASS / don't know what SASS variables are, you shouldn't need to care about this difference. Meaning: most users of the theme will only have some custom.css (at most, if they already use CSS directly), and not also have SASS code like is the case in book-sphinx-theme. And for those users, this note is not relevant (which we thus need to clarify ;))

It's unclear if / how it's possible to use both kinds of variables at the same time

They are using a different syntax in SASS, so normally you can have both, yes. But we don't use SASS variables here, so not fully sure (or no direct experience with it). Here is a bit of docs in SASS about using SASS variables to define CSS variables (I think): https://sass-lang.com/documentation/breaking-changes/css-vars

The CSS variable syntax that this theme uses --default-color: 23, 54, 23; etc, seemed to throw an error with my SCSS compiler. Is this some kind of non-standard syntax?

At which point did it throw an error? Did you use that variable for something in your scss code? And what error did you get?

Where we use those color variables in the scss code in this theme, we always use it with the rgba function, like color: rgba(var(--color-admonition-info), 1);. That ensures that the tuple of three integer values is converted into an actual color code. I thought that rgba(..) only worked by passing those actual tuples of values and not color code, but maybe that's not correct (because otherwise your patch in https://github.com/executablebooks/sphinx-book-theme/pull/246 should not be working ..). That's a gotcha of using the CSS variables in our theme that certainly needs to be documented (it's noted in the theme.css itself which gives the overview of all CSS variables, but can also being mentioned in the rst docs)

jorisvandenbossche commented 4 years ago

because otherwise your patch in executablebooks/sphinx-book-theme#246 should not be working ..

Now, I was looking at the online preview at the PR, but I suppose that that is not using the latest version of the theme (it's pinned at 0.4.1), and so you don't actually see the effect of the CSS variables there .. Did you build the sphinx-book-theme docs locally for that PR using the latest version of pydata-sphinx-theme? And did the colors of the admonitions look correctly?

choldgraf commented 4 years ago

Did you build the sphinx-book-theme docs locally for that PR using the latest version of pydata-sphinx-theme? And did the colors of the admonitions look correctly?

Yeah I think I am being mixed up about versions here...building it locally it seemed to work OK though.

if you are not using SASS / don't know what SASS variables are, you shouldn't need to care about this difference.

I think that's true, though if we want this theme to be used by downstream themes, then we should document this because they may be using SASS or SCSS (like me :-) )

in retrospect I think we may not have fixed this problem in sphinx-book-theme, so maybe I need to wait until a release of the pydata theme comes out before figuring it out...

as an aside, I didn't realize that the "color variables" PR was also changing the default colors...IMO if we change the default colors on people we should also provide documentation on how they can change it back to the defaults because in my experience people just want to keep using whatever colors they were already using

hoetmaaiers commented 4 years ago

This was the exact reason why we chose the non standard color definition. We had to choose between:

I believe this discussion mainly happened in the theme variables PR: https://github.com/pandas-dev/pydata-sphinx-theme/pull/190#discussion_r432924457.

Since this definition is a little less standard, documentation must explain this idea, I agree on that.

Where we use those color variables in the scss code in this theme, we always use it with the rgba function, like color: rgba(var(--color-admonition-info), 1);. That ensures that the tuple of three integer values is converted into an actual color code.

choldgraf commented 4 years ago

Some more confusion - it seems like some admonitions that used to have different colors (e.g. note and tip) now have the same color. I find the mapping of variables onto admonition types to be a little bit confusing.

For example, why does hint admonition have info colors, while tip admonition has warning colors?

More generally, I am wondering why we decided to change the way that colors behaved by default for these admonitions. I think that we should stay with Sphinx default behavior by sticking to the "admonition type -> color mapping" here: https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions

hoetmaaiers commented 4 years ago

I think we tried to stay close to the bootstrap way of defining the variables with is limited to :

Since this doesn't overlap fully with Sphinx these choices were made. Do I remember correctly @jorisvandenbossche ?

jorisvandenbossche commented 4 years ago

So to recap about the admonition colors (sorry, this has become a long post in the end ..). To get an overview, docutils defines those admonitions: attention, caution, danger, error, hint, important, note, tip, warning + the generic admonition. The basic themes don't style much (or only note / warning), but readthedocs styles all of them putting them basically in 4 categories (https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions):

When we changed the style of the admonitions (and to no longer use the bootstrap alert classes for it), we adopted those categories, I think (and also taking the colors of rtd) -> https://github.com/pandas-dev/pydata-sphinx-theme/pull/183 (but for example, mkdocs-material, which was a source of inspiration for the styling, supports more types and uses slightly different colors for almost all of them: https://squidfunk.github.io/mkdocs-material/reference/admonitions/#supported-types).

In the CSS variables PR (#190), Robin initially added a variable to control the color of each of the 9 admonition types, on which I commented that this is maybe overkill (as long as there is no demand on it), and we can keep it to the 4 categories like RTD does (https://github.com/pandas-dev/pydata-sphinx-theme/pull/190#discussion_r432924457). But so then I suggested to use the naming + colors that bootstrap uses for those 4 categories: Warning, Danger, Success, Info (since those colors/categories more or less mapped to the categorization that RTD uses, and we needed to pick some naming scheme for the categories).

Now, a first issue, we currently use those color variables:

So in that conversion, it clearly seems we put some of those in the wrong bucket (so "hint" should not be a warning color, but together with "tip" and "important" in the third category, if we follow the categorization of RTD?). So this is certainly something we can fix up.

(now, it seems that before the variables PR, we also didn't fully follow the RTD categorization ... as "warning" was put in the red error category, and "important" was put in the "note" category and not "hint"/"tip" category)

Then a second issue are the default colors: after #190 we now define 4 color variables --color-success, --color-info, --color-warning and --color-danger, and as default use the default bootstrap color valuer for this (https://getbootstrap.com/docs/4.5/getting-started/theming/#theme-colors). Compared to RTD, I think that gives more or less the same "categories" of colors, with the difference that we use "yellow" instead of "orange" for the "warning" category, and "cyan" instead of "blue" for the "note" category (personally, I am fine with those colors, though. At least I find the "cyan" more pleasant to the eyes as "blue", but that's subjective of course ;)) So we can certainly discuss those default colors! But note that before #190, we were also not using the colors in a similar way as RTD (we were using yellow instead of green for the "tip"/"hint" category).

A third issue is that for the "generic" admonition, we used the "--color-admonition-primary" instead of "--color-admonition-info", while eg RTD uses the same blue as for the "note" for those generic admonitions. That's also something we can certainly change (either by getting rid of the "admonition-primary" color, or by putting it to the same color as "admonition-info".

More generally, I am wondering why we decided to change the way that colors behaved by default for these admonitions. I think that we should stay with Sphinx default behavior by sticking to the "admonition type -> color mapping" here

So after writing all of the above ;-), I think that in the end we didn't really intent to change the way the colors behave (just made some mistakes in convertint to use the variables about which class to use), but just choose the naming scheme of bootstrap for the categories (I am not aware of sphinx having names for those 4 categories?), and thus also change the default colors slightly (we were already using bootstrap colors, but now used other bootstrap colors).

as an aside, I didn't realize that the "color variables" PR was also changing the default colors...IMO if we change the default colors on people we should also provide documentation on how they can change it back to the defaults because in my experience people just want to keep using whatever colors they were already using

Sorry about that, so that's my doing. I should have mentioned that more explicitly. But note it's not yet released, so we can fix it up! ;)

choldgraf commented 4 years ago

Sounds good 👍 thanks for the deep dive! I am personally pretty flexible here, my main concern is that there are thousands of Jupyter Book users, and if the colors of their admonitions suddenly change with an update they are gonna be confused, so whatever we come up with as long as they will not be confused or pissed off at sudden change, I am happy :-)

jorisvandenbossche commented 3 years ago

so whatever we come up with as long as they will not be confused or pissed off at sudden change, I am happy :-)

The problem here is that our original categorization of the different admonition classes was apparently not consistent with RTD. So if we want to make it consistent, there will be a change for some of the admonition types anyway, even if we restore the colors for each category as they are for the released version (see also my last comments at https://github.com/pandas-dev/pydata-sphinx-theme/pull/281).

choldgraf commented 3 years ago

I think that if we can make a principled case for why the new colors are the way they are, then that's fine. But I think that the general approach should be "keep the colors the same, and if something must change, document why that change is being made". What had surprised some folks in Jupyter Book was that suddenly a bunch of colors changed without any pre-warning or justification. I feel like this is sort-of the design equivalent of changing a function without a deprecation cycle and without documenting that it will return a different kind of value or something.