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

[fix] prefers-color-scheme: use dark theme if body[data-theme="auto"] #154

Merged
merged 1 commit into from Mar 16, 2022

Conversation

return42
Copy link
Contributor

@return42 return42 commented Mar 12, 2022

Before this patch the CSS implementation assumes that "dark" is supported by the
theme if the data-theme attribute of the <body> tag is unset.

Since most themes are using "light" colors (compare https://sphinx-themes.org)
and do not set data-theme it is better to assume light is the default, even
when the browser setting prefers-color-scheme: dark!

BTW: remove duplication of styles from dark/light theme that are already set in
the common style.


To test enable dark mode in your browser (prefers-color-scheme: dark).

Current master (default theme):

image

After this patch applied (default theme):

image

I also tested furo theme which supports a dark/light mode. This is the result when browser (prefers-color-scheme: dark):

image

BTW: the result is the same if you toggle from "auto" to "dark".

And here is the result when you toggle into "light" mode ..

image


Closes: #152

Before this patch the CSS implementation assumes that "dark" is supported by the
theme if the `data-theme` attribute of the <body> tag is unset.

Since most themes are using "light" colors (compare https://sphinx-themes.org)
and do not set `data-theme` it is better to assume light is the default, even
when the browser setting `prefers-color-scheme: dark`!

BTW: remove duplication of styles from dark/light theme that are already set in
the common style.

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
return42 added a commit to return42/searxng that referenced this pull request Mar 12, 2022
The myst-parser requires >= docutils v.0.17 what ends in a dependency hell where
plugins sphinx-tabs and sphinx-jinja we use are involved.

This patch can be reverted when [2], [3], [4]  are solved and new release is
available / see [1].

[1] searxng#954
[2] executablebooks/sphinx-tabs#152
[3] executablebooks/sphinx-tabs#153
[4] executablebooks/sphinx-tabs#154

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@return42
Copy link
Contributor Author

@foster999 sorry for being impatient again .. can you merge and release?

Its not only my project also documentation from many other projects are suffered by this issue / e.g. all projects at RTD that are using sphinx-tabs (without pinning an old version) ..

By example, switch your browser to prefer dark mode and visit RTD / e.g.: https://sphinx-notfound-page.readthedocs.io/en/latest/installation.html

grafik

Another example is sphnix-tabs @ RTD itself: https://sphinx-tabs.readthedocs.io/en/latest/

Thanks!

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #154 (f19ff0d) into master (53b6a63) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files           2        2           
  Lines         219      219           
=======================================
  Hits          203      203           
  Misses         16       16           
Flag Coverage Δ
pytests 92.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53b6a63...f19ff0d. Read the comment docs.

@foster999
Copy link
Collaborator

I'm sure you don't need me to tell you that not pinning your dependencies is a bad idea 😉

@foster999 foster999 merged commit 5b2f4ee into executablebooks:master Mar 16, 2022
@return42 return42 deleted the fix-152 branch March 16, 2022 20:11
@return42
Copy link
Contributor Author

return42 commented Mar 16, 2022

I'm sure you don't need me to tell you that not pinning your dependencies is a bad idea

Depends .. pinning a library at major version level is often common .. applications are often pinned at minor level (to get bug fixes) .. but it is handled different in the projects.


EDIT forgot to say / thanks for review & merge!

@return42 return42 restored the fix-152 branch March 16, 2022 20:22
@return42
Copy link
Contributor Author

return42 commented Mar 16, 2022

@foster999 could you deploy a bugfix release (3.3.1) at PyPI .. thanks!

@foster999
Copy link
Collaborator

@return42 new version is released :)

@return42 return42 deleted the fix-152 branch March 18, 2022 17:15
@OlafenwaMoses
Copy link

Thanks very much for this fix @return42

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.

issue with new dark mode of sphinx-tabs in themes that do not support dark mode
3 participants