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

Add dark theme and theme switcher #540

Merged
merged 59 commits into from
Apr 9, 2022
Merged

Add dark theme and theme switcher #540

merged 59 commits into from
Apr 9, 2022

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Jan 7, 2022

Ok so as mentioned in #521 I'm not able to merge the fork I made earlier last year #501 with the new stb method. no problem I redo everything starting from a fresh fork in one of my organization.

description

In this PR I tried to create a dark theme. This dark theme can be triggered by the user preferences and/or using the little btn in the navbar. The user defined value has the priority over the system.

themes-light-dark

design discussion

That's the first time I'm working on CSS so I followed all the recomendation from https://css-tricks.com/a-complete-guide-to-dark-mode-on-the-web/#using-php-cookies.

method

With the index.js I can change the data-theme and data-mode of the <body> tag.
This is changing the behaviour of the css. Every variable is prefixed with a body[data-theme="dark"] to change every time the button is pressed.

coloring

For the coloring I tried to remove all the colors that were defined in the .scss and use variables instead, bringing them all back from the theme.css file to a _color.sccs file. I did a first draft, I'm happy to hear what works and what doesn't according to you.

As a rule of thumb I understand that the colors need to be desaturated so I used this tool : https://mdigi.tools/desaturate-color/#b3571b

pygment

Pygment is an issue as to create 2 themes I need to fix the Pygment css. I used 2 redefined theme (tango for light and native for dark) and overwrite the pygment css file accordingly. The user loose the capacity to change the pygment theming.

shadows

everything that have shadow have been lightened from the dark background to reproduce the depth without the issue of white shadows

update since #501

  • pygment css is generated on the fly so no need to vendor it
  • dark theme is persisting across pages using local storage
  • replace the fancy css btn by a simple icon
  • cleanup in the variables naming

Preview changes

Readthedocs preview is here: https://pydata-sphinx-theme--540.org.readthedocs.build/en/540/

@damianavila damianavila mentioned this pull request Jan 7, 2022
4 tasks
@damianavila
Copy link
Collaborator

@12rambau, FYI I have closed #501 in favor of this one 😉 .

@12rambau
Copy link
Collaborator Author

I have corrected the issue with the documentation, I'll wait for commentsbefore modifying the tests (I've changed the navbar structure, making the test failing)

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.

Sorry for the slow turnaround here - I am pretty new to SCSS light/dark theming as well, so needed some time to understand what's going on.

I think that this PR is a nice step in the right direction. There are a few things that we could clean up and simplify, so I left comments throughout with some suggestions and questions. Happy to keep iterating on this.

In my opinion, we should assume that the theme coloring will take a while to get right, and we should treat this as an experimental feature for the foreseeable future, with no promises to downstream users of stability. That will give us a bit more flexibility to merge and iterate.

src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/assets/scripts/index.js Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/assets/styles/_color.scss Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/assets/scripts/index.js Outdated Show resolved Hide resolved
src/pydata_sphinx_theme/assets/styles/_color.scss Outdated Show resolved Hide resolved
@choldgraf choldgraf changed the title Dark theme (again) Add dark theme and theme switcher Feb 8, 2022
@12rambau
Copy link
Collaborator Author

Sorry - can you just set the SEO assertion minscore to 0.7 ?

done, and now all the tests are passing !

@choldgraf
Copy link
Collaborator

@12rambau I did a quick Lighthouse accessibility audit and found a few things that needed tweaking. Make a PR for your fork here: https://github.com/sepal-contrib/pydata-sphinx-theme/pull/2

In my opinion, once we get the last remaining accessibility things cleared up, we should merge in this PR, and iterate on the colors as-needed in the future.

One thing I'm feeling as I just went through the codebase (and echoing @pradyunsg's thoughts): we have way too many custom-defined color variables. In a subsequent PR, I think we should just define a handful of colors (e.g. "base", "primary", "secondary", etc, and just re-use those throughout). But I think that's for another PR, not this one :-)

choldgraf and others added 2 commits March 29, 2022 16:50
@martinRenou
Copy link

I just want to say thanks for the amazing work on this folks!

@choldgraf
Copy link
Collaborator

@martinRenou if you have other thoughts on how we can improve the dark mode variables / structures / etc please do chime in on follow up PRs or issues! I know you have recently played around with dark modes yourself ;-)

Co-authored-by: Damian Avila <damianavila82@yahoo.com.ar>
Copy link
Collaborator

@damianavila damianavila left a comment

Choose a reason for hiding this comment

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

This is awesome work from @12rambau, thanks for your patience with us!
Approving and merging right away!

@damianavila damianavila merged commit 454918d into pydata:master Apr 9, 2022
@12rambau 12rambau deleted the dark-theme branch April 9, 2022 16:03
@choldgraf
Copy link
Collaborator

wohoo! many thanks @12rambau

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.

6 participants