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

[REVIEW] Fix issues with day & night modes in python docs #11400

Merged
merged 7 commits into from
Aug 1, 2022

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Jul 29, 2022

Description

Fixes: rapidsai/docs#284

This PR fixes day(light) & night(dark) mode color schemes which makes text and a lot of html elements look unclear.

In dark mode:
Before:

Screen Shot 2022-07-28 at 10 01 38 PM

After:
Screen Shot 2022-07-29 at 3 31 54 PM

In light mode:
Before:

Screen Shot 2022-07-28 at 10 03 36 PM

After:
Screen Shot 2022-07-29 at 3 31 07 PM

Introduced darker color schemes such that code text highlightings are visible properly in dark mode:

Before:
Screen Shot 2022-07-28 at 10 06 08 PM

After:
Screen Shot 2022-07-28 at 10 06 15 PM

Introduced custom javascript method that will add hover text to "Theme switcher" button:

Screen Shot 2022-07-28 at 9 59 40 PM

Screen Shot 2022-07-28 at 9 59 28 PM

Checklist

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team doc Documentation non-breaking Non-breaking change labels Jul 29, 2022
@galipremsagar galipremsagar added this to PR-WIP in v22.08 Release via automation Jul 29, 2022
@galipremsagar galipremsagar self-assigned this Jul 29, 2022
@galipremsagar galipremsagar moved this from PR-WIP to PR-Needs review in v22.08 Release Jul 29, 2022
@bdice
Copy link
Contributor

bdice commented Jul 29, 2022

From Mark's screenshot in rapidsai/docs#284, it looks like the dark mode font colors (lighter colors) were being applied but the background color remained light (instead of being dark). Are we overriding something elsewhere in our CSS, or do we need a newer version of the theme? I'm looking into this a bit further, but I am surprised we need to add custom code since dark/light switching appears to be a built-in feature of the theme.

@bdice
Copy link
Contributor

bdice commented Jul 29, 2022

I was not able to reproduce the error in rapidsai/docs#284 where dark mode was not being applied. This screenshot is building from branch-22.10 with pydata-sphinx-theme 0.9.0.

image

@harrism @galipremsagar Is this problem specific to a particular browser or operating system? I used Firefox, Chrome, and Edge and did not see an issue. I varied the light/dark system settings in Windows as well. Is there a problem we need to report upstream, or an existing issue/PR in the upstream theme that this was based on?

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@c11b780). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head da3d576 differs from pull request most recent head a679272. Consider uploading reports for the commit a679272 to get more accurate results

@@               Coverage Diff               @@
##             branch-22.08   #11400   +/-   ##
===============================================
  Coverage                ?   86.47%           
===============================================
  Files                   ?      144           
  Lines                   ?    22856           
  Branches                ?        0           
===============================================
  Hits                    ?    19765           
  Misses                  ?     3091           
  Partials                ?        0           

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 c11b780...a679272. Read the comment docs.

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Jul 29, 2022

I was not able to reproduce the error in rapidsai/docs#284 where dark mode was not being applied. This screenshot is building from branch-22.10 with pydata-sphinx-theme 0.9.0.

image

@harrism @galipremsagar Is this problem specific to a particular browser or operating system? I used Firefox, Chrome, and Edge and did not see an issue. I varied the light/dark system settings in Windows as well. Is there a problem we need to report upstream, or an existing issue/PR in the upstream theme that this was based on?

So I suppose there's a typo in Mark's first comment:

In dark mode, the paragraph font color is light grey on white, which is hard to read.

I think he meant "In light mode", instead.

Are we overriding something elsewhere in our CSS, or do we need a newer version of the theme? I'm looking into this a bit further, but I am surprised we need to add custom code since dark/light switching appears to be a built-in feature of the theme.

Yes, we are overriding certain default color schemes that pydata-sphinx-theme choose to go with. Which is why it is difficult to read in the first place with the default color schemes. I have made changes to the dark theme to be much easier to read for everyone. It is in some ways similar to GitHub dark theme.

Another example of why going with the default color schemes isn't a good idea is this: pydata/pydata-sphinx-theme#649

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Discussed offline. LGTM!

v22.08 Release automation moved this from PR-Needs review to PR-Reviewer approved Jul 29, 2022
@galipremsagar
Copy link
Contributor Author

This PR also requires rapidsai/docs#285 to be merged.

Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the hover as well.

rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Jul 29, 2022
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jul 29, 2022
@@ -1156,7 +1156,7 @@ HTML_HEADER =
# that doxygen normally uses.
# This tag requires that the tag GENERATE_HTML is set to YES.

HTML_FOOTER =
HTML_FOOTER = footer.html

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to blank out the filename in this line: HTML_EXTRA_STYLESHEET = rapids.css
Since you've delete the rapids.css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch, David. Deleted this.

@@ -0,0 +1,3 @@
<script src="https://docs.rapids.ai/assets/js/custom.js"></script>
<link rel="stylesheet" href="https://docs.rapids.ai/assets/css/custom.css">

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Do these files not need a copyright header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, added now.

v22.08 Release automation moved this from PR-Reviewer approved to PR-Needs review Jul 29, 2022
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

@harrism
Copy link
Member

harrism commented Jul 29, 2022

In dark mode, I find the purple headings a bit hard to read on a black background (but I'm sitting in a bright room). We should probably evaluate this in a follow-up. For now, thanks for the fixes!

rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Jul 29, 2022
rapids-bot bot pushed a commit to rapidsai/cucim that referenced this pull request Jul 30, 2022
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this pull request Aug 1, 2022
Fixes similar issue found in: rapidsai/cudf#11400

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Mads R. B. Kristensen (https://github.com/madsbk)

URL: #95
@shwina
Copy link
Contributor

shwina commented Aug 1, 2022

rerun tests

@shwina
Copy link
Contributor

shwina commented Aug 1, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6e0e47e into rapidsai:branch-22.08 Aug 1, 2022
v22.08 Release automation moved this from PR-Reviewer approved to Done Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team doc Documentation libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[BUG] Dark mode font color is too light
5 participants