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

Support dark mode #635

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Feb 23, 2022

in your furo theme branch, you don’t style data frames in a way that’s readable on a dark background:

this is how this page looks for me by default https://nbsphinx.readthedocs.io/en/furo-theme/code-cells.html:

grafik

after:

grafik

I don’t know if other themes’ dark modes

@mgeier
Copy link
Member

mgeier commented Feb 24, 2022

Thanks for this PR!

I'm not yet familiar with dark mode, I'll have to read up a bit.

But I already have a question: How does JupyterLab (which also has a dark theme) handle this situation?
In a similar way?

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Feb 25, 2022

The general problem is that while many things are able to use the system theme by default (prefers-color-scheme: dark), they also allow some toggle to override this. And the way this manually selected state is stored depends on the theme / app author.

@adamgayoso
Copy link

Btw this PR will not work when user prefers dark, so it needs to do something like not theme="light"

see scverse/scvi-tools#1448

@flying-sheep
Copy link
Contributor Author

Why not? You’re doing body:not([data-theme=light]) <selector> and I’m doing *[data-theme="dark"] <selector>. Both should work right?

@adamgayoso
Copy link

not exactly, at least it didn't for me when your browser "prefers dark"

(not night mode on furo, but this is the "default", prefers logo)
Screen Shot 2022-03-18 at 7 27 33 AM

@ohrely
Copy link

ohrely commented Mar 19, 2022

I have also run into this, needed a solution quickly so I overrode the hardcoded colors in https://github.com/spatialaudio/nbsphinx/blob/master/src/nbsphinx.py#L658 with theme-agnostic CSS variables from Jupyter:

.jp-RenderedHTMLCommon table,
div.rendered_html table {
  color: var(--jp-ui-font-color1) !important;
}
.jp-RenderedHTMLCommon thead,
div.rendered_html thead {
  border-bottom: var(--jp-border-width) solid var(--jp-border-color1) !important;
}
.jp-RenderedHTMLCommon tbody tr:nth-child(odd),
div.rendered_html tbody tr:nth-child(odd) {
  background: var(--jp-layout-color0) !important;
}
.jp-RenderedHTMLCommon tbody tr:nth-child(even),
div.rendered_html tbody tr:nth-child(even) {
  background: var(--jp-rendermime-table-row-background) !important;
}
.jp-RenderedHTMLCommon tbody tr:hover,
div.rendered_html tbody tr:hover {
  background: var(--jp-rendermime-table-row-hover-background) !important;
} 

ETA Would it be easier to unbreak Furo dark mode by removing the explicit color values introduced in #228?

@flying-sheep
Copy link
Contributor Author

I think I have an approach that should work.

@flying-sheep
Copy link
Contributor Author

Could you take a look please?

@tmke8
Copy link

tmke8 commented Jun 13, 2022

By the way, stderr is also a bit hard to read:
image

Copy link
Contributor

@2bndy5 2bndy5 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 a good start to solving #734, but it won't fully resolve the issue.

--nbsphinx-table-row-odd-bg: rgba(255, 255, 255, .1);
}
}
body[data-theme="light"]) {
Copy link
Contributor

@2bndy5 2bndy5 May 3, 2023

Choose a reason for hiding this comment

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

data-theme is not a standard attribute for body elements. Some sites use a site-specific attribute name to control light/dark mode toggling in site-specific JS. I think furo specifically uses this attribute name, so this may have unintended consequences with other themes that don't (or do) support dark mode.

The sphinx-immaterial is a port of the mkdocs-material theme, and the attribute similar to furo's data-theme is data-md-color-scheme="slate" for dark mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, I think :root is preferred over body in CSS selectors for setting variables. This is just based on my various experiences.

div.rendered_html tbody tr:hover {
@media (prefers-color-scheme: dark) {
body:not([data-theme="light"]) {
--nbsphinx-table-row-odd-bg: rgba(255, 255, 255, .1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This color (rgba(255, 255, 255, .1)) is essentially a shade of grey in dark mode. It might make more sense to use rgba(127, 127, 127, 0.25) for both light and dark modes. That way the foreground color doesn't also need compensation. Font colors are usually more absolutely white or black, but colored text will depend more on the saturation of the color used in text; this discrepancy doesn't arise often.

I tested my color suggestion in furo, and the text is still legible.
image image
The suggested alpha value could be adjusted (maybe lesser) to reduce the subtlety though.


While this comment is focused on the CSS selector tr.row-odd (or something more specifically used by nbsphinx), it could also be applied to the input code-block's added border. I have listed all the problematic hard-coded colors in #734 (comment)

@12rambau
Copy link

Just to add some information about the dark-themes in general, in pydata-sphinx-theme we decided to endorse the support of the dark-theme display so we do customise the redenring of supported libs to match the theme color scheme.

https://github.com/pydata/pydata-sphinx-theme/tree/main/src/pydata_sphinx_theme/assets/styles/extensions

As it's very much theme related, I'm not sure that coloring should be a responsability of nbsphinx itself.

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.

None yet

7 participants