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.

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

Updating note color and adding docs for colors #281

Merged
merged 8 commits into from
Jan 19, 2021

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Nov 4, 2020

Related to #279 - this does two things:

  1. Updates the note admonition so that it is the same color as the default admonition (this is what RTD does)
  2. Updates the documentation around custom CSS and variables so that it's a bit easier to follow

Here are the admonitions for comparison: https://pydata-sphinx-theme--281.org.readthedocs.build/en/281/demo/demo.html#admonitions

Here are the updated docs: https://pydata-sphinx-theme--281.org.readthedocs.build/en/281/user_guide/customizing.html#custom-css-stylesheets

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this PR now, after my way too long dive into the topic and resulting topic at #279 (comment) ;)

But thanks for improving those docs!

@@ -125,12 +125,12 @@
}

&.tip {
border-color: rgba(var(--color-admonition-info), 1);
border-color: rgba(var(--color-admonition-warning), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the categorization of RTD, this should be "success" I think, instead.

And so the "hint" just above, should not use "warning", but also "success"

@@ -149,12 +149,12 @@
}

&.note {
border-color: rgba(var(--color-admonition-info), 1);
border-color: rgba(var(--color-admonition-primary), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we keep this at "info" category, but so we should maybe combine the info/primary categories (or at least give them the same colors, see the "third issue" in #279 (comment))


These are based on top of the basic `Bootstrap CSS variables <https://getbootstrap.com/docs/4.0/getting-started/theming/#css-variables>`_
extended with some theme specific variables. An overview of all variables and
every default is defined in `the pydata default CSS variables file <pydata-css-variables_>`_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for updating this text. A link to the actual file with the variables is quite crucial to know which variables are available to override ..


Note that these are `CSS variables <css-variable-help_>`_ and not
`SASS variables <https://sass-lang.com/documentation/variables>`_.
For the difference between the two, see he theme is defined with CSS variables,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something wrong in this sentence I think

@jorisvandenbossche
Copy link
Member

@choldgraf I updated the colors of the different admonition types to now use a consistent categorization. So the demo of this PR: https://pydata-sphinx-theme--281.org.readthedocs.build/en/281/demo/demo.html#admonitions now should show the same grouping as on the RTD demo (https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions)

That solves the first issue of #279, but of course still leaves open the two other questions of:

  • The exact color to use for each group. Do we want to take colors closer to RTD (basically what you did in the original version of adding custom admonitions classes #183), or are we fine with bootstrap success/info colors?
  • Which group/color to use as a default when no specific class is used (when "making your own admonition" without specifying a class. Currently this is using the "primary" color of the theme, which is also used for the navigation highlighting, but we could also have it the same as the "note" class (which is what RTD does).

@jorisvandenbossche
Copy link
Member

For the default color question, doing a quick comparison (with a slightly updated demo.rst locally to just have one admonition of each category).

The current colors (using bootstrap theme colors):

image

Using RTD's colors (but with our logic of using 0.1 alpha for the lighter background of the title, so this gives a lighter impression as on RTD):

image

And thanks to the CSS variables, the above is only a small change:

--- a/pydata_sphinx_theme/static/css/theme.css
+++ b/pydata_sphinx_theme/static/css/theme.css
@@ -27,10 +27,10 @@
   * Colors are defined in rgb string way, "red, green, blue"
   **/
   --color-primary: 19, 6, 84;
-  --color-success: 40, 167, 69;
-  --color-info: 23, 162, 184;
-  --color-warning: 255, 193, 7;
-  --color-danger: 220, 53, 69;
+  --color-success: 26, 188, 156;
+  --color-info: 106, 176, 222;
+  --color-warning: 240, 179, 126;
+  --color-danger: 242, 159, 151;
   --color-text-base: 51, 51, 51;

So this are then the colors you had in the original commit in #183, eg as in the screenshot in the top post of that PR.
As mentioned above, it only looks "lighter" because we are now using a fixed alpha of 0.1 instead of allowing to define this color manually as well (we should maybe add a variable for this as well?)

Using an alpha of 0.2 instead of 0.1, I get:

image

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Nov 19, 2020

And as reference, here is how it currently looks with the released version 0.4.1 (only with one additional admonition, to show the 4th color, because "warning" was using the "wrong" color):

image

So we could restore some of those colors to keep them as the default in the upcoming release as well (for example, the "blue" for the notes), but I think having both the yellow, orange and red shades is maybe not something we should keep (only two shades like orange and red, or yellow and red, seems sufficient, so there is more variation in colors?)

@choldgraf
Copy link
Collaborator Author

Two quick thoughts -

  1. I don't feel super strongly about all the specific colors, though I think that the most-common admonition is probably "note" and that we should probably keep ~ the same, so maybe that's the minimal thing to keep?
  2. One reason I like the yellow for "hint" is because the lightbulb icon naturally pairs well with the "yellow" as in "the color of the lightbulb"...that's one reason the green feels weird to me there.

@jorisvandenbossche jorisvandenbossche changed the title updating note color and adding docs for colors Updating note color and adding docs for colors Dec 29, 2020
@jorisvandenbossche
Copy link
Member

So I pushed:

  • added variables for all admonition types, so you have more control (eg if you want to update the color for just the hint admonition). At the same time, I also made variables for the icons.
  • updated the color used for the "primary" group (note + the generic admonition) to the bright blue from bootstrap that was used before for the note admonition

Copy link
Collaborator Author

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems good to me 👍 thanks for pushing to the PR! Feel free to merge if you are good w/ it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants