-
-
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
Dark Mode #4448
Comments
What can anyone do? I think I might've brought this up too somewhere. Mind posting the 2021 staff roadmap somewhere, so people don't waste their time on lower priorities? |
Hello, me and my collegue @dbouris are interested in this issue. We can try to apply the css and the javascript needed, in order to make a full functional dark mode. Is it posssible to assign this issue to us? |
hey @mekarpeles and @jimchamp! I'd love to pick this up if it's still pending if you'd like to assign me? |
I'm not even sure that there's enough information here to even get started with this. At a minimum (and to start with), we'll want some mock-ups for dark mode views (including the dark mode toggle, which is commonly a moon or sun icon). |
Hi @jimchamp, that's a very good point & makes complete sense. For the initial mock-ups, is that more a job for the designers? |
Thank you @jimchamp and @DarrellRoberts. I think for someone who wants to take this challenge on, commenting on this issue with a minimum plan would be useful, e.g.:
Mock-ups would also be useful. It doesn't have to be perfect, even demonstrating how much work this project might take would be a valuable learning. |
thank you @mekarpeles ! I can work on the minimum plan answering your bullet points, along with screenshots, if that sounds good? I've created dark mode for other projects before though only on JavaScript frameworks but up for a challenge. p.s. apologies for missing today's call. I got my timezones mixed up. |
hey @jimchamp and @mekarpeles, see below for my proposed action plan for implementing dark mode. In a nutshell, my thinking was that we have three CSS Less mixins: lightmode(), darkmode() and toggletheme(). The toggletheme() mixin is then used for the background colors and other colors for the body element along with its children. A data-theme is added to the body element with JavaScript, telling the toggle-theme mixin whether it is "light" or "dark". The button then allows the user to change this state. I used localstorage for watching this state but I'm open to suggestions if you think there's a better way. Please note, at this stage I'm not sure where the dark mode button should go nor what the design should look like. Nevertheless, I've provided screenshots at the end of a possible design for the dark mode (used Chrome's automatic dark mode dev tools), though we may want a more desaturated look, but maybe this is more of a job for the designers? Let me know what you think. Dark Mode ImplementationCSS
.light-theme() will take the default values.
e.g. (line 39)
HTML
JavaScript
I added two functions to the index.js because I want to first initialise the theme as well as toggle the theme. This is what I added to the openlibrary/plugins/openlibrary/js/index.js file
(line 561)
Line 550 binds the initial theme to the body element. Line 561) adds a click event listener to the toggle button
This states then upon loading the page, the data-theme will be taken from the local storage, which is where it is stored. If there is no theme, it reverts to the default: 'light'. By doing this, I allow the theme to remain dark on a page refresh or a new page load.
Following on from the local state, the toggle changes both the data-theme within the body element as well as in the local storage. As the mixins are conditional and listen for the data-theme, changing the data-theme affects the body element and all of its children. And that's it. ScreenshotsFor the button I just used a simple HTML element and positioned it next to the logo. This isn't what I'm proposing in terms of the button's design or position and it was just easier to test. As for the actual design of the button, I would usually use a moon/sunshine icon. For its position, I was thinking the top-right in the navbar but I'm completely open to feedback. |
Sorry for the delay, @DarrellRoberts, and thanks for putting this together. I don't think that we need to use a data attribute for this. Just add a new class to the I'm not sure that I understand the purpose of the Storing the dark mode settings exclusively in We'll want to get some feedback from the design community on the following:
In the meantime, it would probably be useful if a small proof of concept was created for this feature. Maybe something that only toggles the background color of the site from beige to something darker when dark mode is selected on the settings page? I just want to make sure that this can be implemented without a jarring theme change on page load. |
Hi @jimchamp , thanks so much for your feedback! Regarding your points:
Sure thing, that makes better sense.
Originally I thought I needed a condition and the toggleTheme() mixin was a way of switching between the two styles. However, after revisiting it, you're right that we just need a .dark CSS class and I was overcomplicating it.
Sure thing, although I'm having difficulty retrieving the cookie value in the /site/body.html. I could either set the cookie via JavaScript DOM or in Python in the plugins/openlibrary/home.py file (please say if this is the wrong file to do it). e.g.
However, when it comes to retrieving the value, I'm not sure where/how to do it and how I can pass it to the /site/body.html template (where, once retrieving the value, I could conditionally append() a 'dark-theme' to the $bodyclass). I believe this is the method: https://webpy.org/cookbook/cookies, but I'm not sure how nor where to execute it. Would you be able to give some direction?
Sure thing, I'll post a message and link to this issue
Sure thing, once I figure out how to retrieve the cookie value on the /site/body.html, I can submit a PR. Everything else works fine. |
Sorry about that! I thought that cookies were exposed to template files, but they are not. Adding something like the following to
The |
no worries, thank you! |
I think
|
Originally proposed by @vidhi-mody in #3259 -- this is a community requested feature which is not on the 2021 staff roadmap, but we imagine folks will be happy about this change, so we're happy to guide the community to implement this feature (lower priority)
The text was updated successfully, but these errors were encountered: