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

ENH: simplify color variables #659

Merged
merged 23 commits into from May 25, 2022
Merged

ENH: simplify color variables #659

merged 23 commits into from May 25, 2022

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented May 9, 2022

summary

Reduction of the color variables from 79 to 17, respecting the guidelines from material colors.

  • classic 7 colors (primary ... danger) palette
  • 2 text colors (base, muted)
  • 4 depth levels: background, on-background, surface, on-surface (in depth order)
  • 1 border and 1 shadow
  • I kept the inline code color (it's kinda the theme signature to me)
  • I kept the custom link color as well
  • I kept the target color as it's well designed and never conflicts with anything

FIx #623, Fix #279

customization

These modifications required some adjustments, in the directive color scheme:

question

Why is there a transition in admonition css (https://pydata-sphinx-theme--659.org.readthedocs.build/en/659/demo/kitchen-sink/paragraph-markup.html#admonitions) ? It creates a funny behavior when switching from light to dark theme (ref). Let me know if you think it should be removed

finally

What do you think ?

@12rambau 12rambau marked this pull request as ready for review May 9, 2022 22:50
Copy link
Collaborator

@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 looks really nice - in general the style seems to still look good (IMO) and I really like reducing the number of configuration options to expose.

a few quick thoughts from me:

Header links are in danger color

Why not make these the same color as other links on the page? That way link style within the page is the same for all elements.

Why is there a transition in admonition css

I did a quick look and it seemed fine to me - note that most people are not going to be regularly switching light/dark mode, so IMO it is OK.

code block CSS

there doesn't seem to be any background or shadow for code blocks, which makes it feel much more "flat" on dark mode:

chrome_KXlm9IhOdY

variable naming

What do you think about switching the variable prefix from --pst to --theme. That would make it a bit less clunky to sub-theme this one and extend or add your own variables. We could do a deprecation cycle to keep the --pst variables around if we wanted to.

@12rambau
Copy link
Collaborator Author

Why not make these the same color as other links on the page? That way link style within the page is the same for all elements.

Because the titles are already blue (primary) so it would be confusing From what I read in the issues, the Pandas theme will be progressively dropped so titles will be blue in both themes eventually. I like the contrast between these 2 items.

Why is there a transition in admonition css ?

My question was more, "I think it's useless can we remove it ?". As you mentioned people are rarely jumping from light to dark so I think this is some legacy code from the inspiration.

What do you think about switching the variable prefix from --pst to --theme?

I actually like --pst- it's easily identified, makes sure that nobody is going to use the same one and overwrite ours by accident, and it's shorter to write than "theme". What is the use case you have in mind ?

@choldgraf
Copy link
Collaborator

I think it's useless can we remove it ?

Ah yeah - I thin that's fine

What is the use case you have in mind ?

My use case is themes that sub-theme this one. In my case, the sphinx book theme (https://sphinx-book-theme.readthedocs.io/). It feels weird to ask users to define some variables as --pst- and others as --sbt-. Though I guess now that the number of unique variables is much smaller, it wouldn't be so bad to just copy each of them over to an --sbt- value.

@choldgraf
Copy link
Collaborator

Anything else you're hoping to tackle in this one? Or are we just waiting for approval and merge @12rambau ?

One quick thing: can we update the docs on the theme variables page so it's clearer which variables are the ones we'd like people to update?

@12rambau
Copy link
Collaborator Author

I've updated the documentation with color variables.

question: Is it in normal that the other variables are still stored in _static and not in style/_base ?

@choldgraf
Copy link
Collaborator

I made some quick updates to the docs, but they look good to me!

I am not sure why the other variables are in the static folder instead of assets with the other CSS variables / rules. My preference would be to keep them all together, rather than have some in assets and others in static. I think that @pradyunsg elected to leave the variables in static when he converted over the theme builder infrastructure in:

@pradyunsg I couldn't find a rationale for leaving some variables in the static folder, any chance you could provide guidance here? Would you object to having all CSS variables in assets and use static only for programmatically generated files?

@jarrodmillman
Copy link
Collaborator

@12rambau Should we merge this before releasing 0.9rc1 later today? If so, would you mind resolving the merge conflicts?

@jarrodmillman jarrodmillman added this to the 0.9 milestone May 25, 2022
@choldgraf
Copy link
Collaborator

I'm happy with @jarrodmillman's suggestion in #675 to:

  • Open an issue to discuss and decide on how to standardize the CSS variables
  • Merge this in and iterate from there

@12rambau
Copy link
Collaborator Author

I resolved the conflicts in the GitHub editor, I hope linter will not complain. But after that we are good to merge

@choldgraf
Copy link
Collaborator

issue to track moving the color CSS variables: #676

@12rambau
Copy link
Collaborator Author

12rambau commented May 25, 2022

DO NOT merge, the navbar is transparent.

I'm correcting it and merging as I have already 2 approvals

@jarrodmillman
Copy link
Collaborator

Good catch! I didn't even notice when I scrolled down the page.

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.

Simplify CSS variable naming / structure Better documentation for how colors are configured
3 participants