-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Testing Dark Mode feature on settings #9231
Testing Dark Mode feature on settings #9231
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DarrellRoberts! This seems to be working pretty smoothly. I've changed the base branch from master
to dark-mode
, as we'll still need to update the stylesheets after the final designs and color scheme have been created and approved.
I've added a number of suggested JS changes. Once those are merged, I'll merge this into the dark-mode
branch.
if (darkMode) { | ||
module.initDarkMode(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (darkMode) { | |
module.initDarkMode(); | |
} | |
module.initDarkMode(darkMode); |
We can be sure that darkMode
is true
at this point.
Add event listeners to darkMode
in initDarkMode
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my last comment, I'll update this shortly. Thank you for reviewing it!
|
||
// Dark Mode toggle | ||
const darkMode = document.getElementById('darkMode') | ||
darkMode.addEventListener('click', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
darkMode
may be null
at this point (it only exists on the settings page). It would be better to pass darkMode
as a parameter of initDarkMode
, and add the event listener in the body of initDarkMode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for highlighting this, I can see that this is how the other .js in structured in index.js. I'll submit the updated PR shortly
const dm = 'dm' | ||
const darkModeValue ='True'; | ||
const cookieStr = `${dm}=${encodeURIComponent(darkModeValue)}`; | ||
document.cookie = cookieStr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const dm = 'dm' | |
const darkModeValue ='True'; | |
const cookieStr = `${dm}=${encodeURIComponent(darkModeValue)}`; | |
document.cookie = cookieStr; | |
document.cookie = 'dm=True;'; |
I don't think that encodeURIComponent
is needed here. This function is more useful for modifying URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Jim! I wasn't aware that it was much simpler to change the cookie
…ode, made darkmode a parameter)
…ed cookie code, use darkMode as parameter)
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @DarrellRoberts!
Supports #4448
This is a proof of concept for the dark mode feature as discussed on issue #4448.
Currently it just changes the body background from its default beige to dark beige. This is also stored in a cookie value, so the state will remain after changes to the web page or a refresh.
Technical
I used JavaScript to set the cookie value when the user clicks the button. This is in a new file called openlibrary/plugins/openlibrary/js/darkMode.js.
Then I retrieve the cookie value with the Python method in openlibrary/plugins/upstream/utils.py and use an if statement in the body in openlibrary/templates/site/body.html.
The button is rendered on the settings page in the file openlibrary/templates/account.html
I used simple images for the light and dark icons, but this is just for the test version and I'm not proposing this for the final design.
Testing
Go to the settings page, click on the button with a crescent moon on it at the bottom. Check that the color of the background changes and try refreshing the page or navigating elsewhere to see if the state remain. Also check the cookies for a cookie called "dm" which should have the value "True" when the button has first been clicked. The button in dark mode should change to a sun icon. On clicking the sun icon, the "dm" cookie value should become empty and the background should return to normal.
Screenshot
Stakeholders
@jimchamp