Skip to content

Reconsider dark mode default behavior #684

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

Closed
philippjfr opened this issue May 28, 2022 · 21 comments
Closed

Reconsider dark mode default behavior #684

philippjfr opened this issue May 28, 2022 · 21 comments
Labels
impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship
Milestone

Comments

@philippjfr
Copy link
Contributor

philippjfr commented May 28, 2022

I really love, love, love the work on the dark theme (and pydata-sphinx-theme in general) but having just tried it on some of our websites I'm wondering whether adding the toggle (and the default_mode: 'auto' behavior) should be opt-in by default. What I found was that since my system is set to dark mode it automatically switched the website to dark mode on render.

Now the dark mode probably works fabulously for websites that consist primarily of text and code, but as soon as you have any rich output such as pictures, plots, etc. the dark mode is visually very painful because in most cases these contents cannot easily be adapted to accommodate both modes. Let's take a matplotlib plot with transparent background as example:

Screen Shot 2022-05-28 at 16 43 05

Not only is the white on black contrast glaring but the axis labels are basically invisible. Any MathJax equations presumably would have the same issue and become basically invisible (although I have not tested this). Similarly now let's look at some bokeh content:

Screen Shot 2022-05-28 at 16 44 49

Now you could say that this is our job to ensure that bokeh correctly toggles themes based on the media query. That's possible true but practically it means that while this is not handled any bokeh output will look terrible when rendered with v0.9.

Since adapting all the contents to render nicely in dark mode will take many library authors some time, I would suggest that at least initially the dark theme switch and the auto default behavior be opt-in. Alternatively, I would suggest documenting how to disable them easily, as this required some digging in the source code:

html_theme_options = {
...
    "navbar_end": ["navbar-icon-links"],
}

html_context.update({
    "default_mode": "light"
})

and causes errors in the pydata_sphinx_theme JS since the switch can't be found.

@drammock
Copy link
Collaborator

+1

I've also noticed that there is apparently no memory of a user's dark mode setting? E.g., if I manually change from auto to light mode, that choice doesn't persist through closing/re-opening the browser (though maybe that should be a separate issue)

@12rambau
Copy link
Collaborator

@philippjfr first thanks for your nice comments, then I would like to add some prescision to this sitution:

  • The dark theme is now using a simpler set of variable making it way easier to update from your side (ENH: simplify color variables #659)
  • we are also supporting some common libs like sphinx-panels (Add panels support #641) to aline to other themes like Furo
  • Since images can be an issue we added the only-dark and only-ligh class that can help you display the rich content in only one of the theme.
  • I'll soon be adding this behaviour for the logos (Support a logo for the dark theme #674)

That being said I realize that's a lot of work on documentation maintainers so yes the auto mode shouldn't be opt-in by default. I have added this "default_mode" parameter back in #540. Sorry if I add forgotten to document it I'll do it ASAP.

@drammock the selected mode is saved in LocalStorage and I add the feeling that was sufficient on my computer I'll check again.

@philippjfr
Copy link
Contributor Author

Thanks for the quick and detailed reply! I've had some issue with getting the default_mode working consistently for me but I suspect that is user error (or rather I guess it got overridden by the setting that was already persisted to LocalStorage).

On the only-light and only-dark CSS classes that is indeed helpful but the problem we're having is that we generate almost all of our docs from notebooks. So now we'd have to generate two versions of the output (at least for stuff that generates images) and for bokeh plots we will somehow have to figure out if we can apply a theme conditionally based on the media query. So for now I would suggest that there be some way to disable the toggle cleanly as well.

@12rambau
Copy link
Collaborator

You could use "light" as default_mode and remove the theme_switcher from the navbar and you should be good to go isn't it ?

html_theme_options = {
   #...,
   "navbar_end": ["navbar-icon-links"]
}

@philippjfr
Copy link
Contributor Author

Removing the theme switcher like that seems to cause some JS errors which show up in the console.

@12rambau
Copy link
Collaborator

didn't think about that. we should silence the js if the theme-switcher is nowhere to be found on the page

@12rambau 12rambau mentioned this issue Jun 3, 2022
12 tasks
@choldgraf choldgraf added the impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship label Jun 3, 2022
@choldgraf
Copy link
Collaborator

choldgraf commented Jun 3, 2022

Hey all - I thanks @philippjfr for sharing your experience. I can see how it'd be frustrating to have all the nicely-generated image outputs suddenly get lost in blackness 😄

So it sounds like the steps to close this are:

  • Confirm that default_mode works as expected, and @philippjfr please provide feedback on how docs could be better, IMO user error can almost always be improved with better docs!
  • Double check there aren't errors that pop up when the switcher is removed from the navbar.

I added a block-release label to this one since it sounds like there are actually some bugs in the toggler behavior

@jarrodmillman
Copy link
Collaborator

jarrodmillman commented Jun 8, 2022

@choldgraf @12rambau I missed this and made the 0.9.0 release. Is that OK? Should I yank it from PyPI? I am also happy to make a 0.9.1 or 0.10.0 soon if that would help.

@12rambau
Copy link
Collaborator

12rambau commented Jun 8, 2022

IMO it's fixed by #692 but I'll let @philippjfr confirm

@philippjfr
Copy link
Contributor Author

Running a documentation build on our CI now, will report back when it's done.

@choldgraf
Copy link
Collaborator

If we need to change something I'd vote for a patch release relatively quickly

@choldgraf
Copy link
Collaborator

I also think we may not have updated the dropdown version?

image

we should get that into the patch release too

@jarrodmillman
Copy link
Collaborator

I am happy to make a new release whenever it is ready.

@jarrodmillman jarrodmillman modified the milestones: 0.9, 0.9.1 Jun 8, 2022
@choldgraf
Copy link
Collaborator

One other quick thought here, since I suspect that outputs like matplotlib, bokeh, etc are particularly important given that this theme is "for" the pydata community. I think we could also explore adding some CSS specifically for the pydata libraries we want to support, so that their outputs still look good on the dark theme. Eg maybe for matplotlib plots we keep the background light to ensure that the tickmarks don't disappear. Do others agree?

@philippjfr
Copy link
Contributor Author

Can confirm this is fixed. I'd be happy for this to be closed now and create a separate issue for carrying on the discussion about handling common output in dark mode.

@12rambau
Copy link
Collaborator

12rambau commented Jun 8, 2022

@choldgraf i aggree with your suggestion, for the sake of archiving, I moved your comment to another issue so that we can follow-up there (#706)

@tupui
Copy link
Contributor

tupui commented Jun 9, 2022

One other quick thought here, since I suspect that outputs like matplotlib, bokeh, etc are particularly important given that this theme is "for" the pydata community. I think we could also explore adding some CSS specifically for the pydata libraries we want to support, so that their outputs still look good on the dark theme. Eg maybe for matplotlib plots we keep the background light to ensure that the tickmarks don't disappear. Do others agree?

@choldgraf I've updated the theme for SciPy and also noticed this. I made some quick experiment and at least for SciPy doing this would work:

  • png (basically Matplotlib output): use invert(0.82) (0.82 to blend, I suppose this is linked to the contrast being 1.2). It looks great IMO.
  • .MathJax_SVG * (mathjax output): also tweak with fill: rgb(250, 250, 250)
  • jpg (we can assume, and document, these are normal images): only some grayscale adjustments to have less vibrant images.

I can make a PR if wanted. Meanwhile, I will make a PR in SciPy I think.

p.s. nice reading https://web.dev/prefers-color-scheme

@mwaskom
Copy link

mwaskom commented Jun 9, 2022

Color inversion is a good trick for libraries that use simple plots to illustrate numerical operations, but it's not great for plotting libraries, i.e it's very confusing to document setting the color of an element to blue and having it show up as orange.

Another risk with the "auto" default is that a developer who does not use dark mode locally might not even realize this is going to happen (of course, they should have read the release notes carefully, but you know, who has the time).

Also, I don't know if this is expected and I am just a web dummy, but it took up a lot of my time figuring out: if you have html_context = {"default_mode": "light"} and look at the site in your browser with file:///, it doesn't actually take effect — you need to actually be serving the site (locally) for it to take effect.

@tupui
Copy link
Contributor

tupui commented Jun 9, 2022

Color inversion is a good trick for libraries that use simple plots to illustrate numerical operations, but it's not great for plotting libraries, i.e it's very confusing to document setting the color of an element to blue and having it show up as orange.

I partially agree because ideally you should try not to rely on colours to explain something because it's less accessible for colour deficient viewers.

Also, I don't know if this is expected and I am just a web dummy, but it took up a lot of my time figuring out: if you have html_context = {"default_mode"} and look at the site in your browser with file:///, it doesn't actually take effect — you need to actually be serving the site (locally) for it to take effect.

Not sure I understand what you mean.

@mwaskom
Copy link

mwaskom commented Jun 9, 2022

I partially agree because ideally you should try not to rely on colours to explain something because it's less accessible for colour deficient viewers.

Right but if you are trying to explain how to make a blue line it is somewhat important for the line to actually appear blue.

Not sure I understand what you mean.

I mean the difference between opening the .html file in your browser vs doing python -m http.server:

image

I have html_context = {"default_mode": "light"} set and this is what I see.

@tupui
Copy link
Contributor

tupui commented Jun 9, 2022

Sure if it's for Seaborn and alike I get that you really care about that.

I suppose the outcome of our discussions should be to find good default for most projects (PyData stack) while explaining how to go around. A simple trick in your case would be to have a specific naming convention to invert or not or do something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship
Projects
None yet
Development

No branches or pull requests

7 participants