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

Testing Dark Mode feature on settings #9231

Open
wants to merge 2 commits into
base: dark-mode
Choose a base branch
from

Conversation

DarrellRoberts
Copy link
Contributor

@DarrellRoberts DarrellRoberts commented May 7, 2024

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

image

image

image

image

Stakeholders

@jimchamp

@DarrellRoberts DarrellRoberts changed the title issue 4448: testing dm feature on settings Testing Dark Mode feature on settings May 7, 2024
@jimchamp jimchamp changed the base branch from master to dark-mode May 30, 2024 17:40
Copy link
Collaborator

@jimchamp jimchamp left a 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.

Comment on lines +574 to +576
if (darkMode) {
module.initDarkMode();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.


// Dark Mode toggle
const darkMode = document.getElementById('darkMode')
darkMode.addEventListener('click', function() {
Copy link
Collaborator

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.

Comment on lines +6 to +9
const dm = 'dm'
const darkModeValue ='True';
const cookieStr = `${dm}=${encodeURIComponent(darkModeValue)}`;
document.cookie = cookieStr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

@jimchamp jimchamp mentioned this pull request May 30, 2024
@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dark Mode
2 participants