-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add Dark Mode #1490
Add Dark Mode #1490
Conversation
- init commit - add darkmode.css file and include
- add darkmode icon - add darkmmode JS file after body
- futher changes to dark mode CSS for index - futher changes to dark mode JS for index
- add hover state styles - complete most of index page
- Get page layout script tag path to work - clean out CSS to nest under body.dark-mode
- add styles for hello-world, routing, generator - hello-world not possible to style iframe
- complete all CSS for all Getting Started pages
- add all script tags - finish JS file - finish all CSS for pages
- fix whitespace
- add css to search bar drop down - rearrange css file and add comments
- add check for local storage
- add hover color on search bar results - fix search bar width on FR, DE, RU, UZ pages caused by adding moon icon to navbar
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.
@chrisdel101 Add system as an option as well and set it as default. Auto detect what's set at OS level and use it as default unless user switches it to either as per their preferences.
What do you mean by Once I understand what you mean I can make these changes. |
You mean like this? https://www.smashingmagazine.com/2024/03/setting-persisting-color-scheme-preferences-css-javascript/ |
- remove error line css - add system style check - remove local storage use until toggled
js/theme.js
Outdated
// remove icon - cannot turn off | ||
document.querySelector('#theme-icon-container').remove() | ||
} else { | ||
// no use local storage til required |
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.
This approach does not utilize local storage at all until the toggle is clicked. I though this would be better.
The code is messier as a result though.
It'd be cleaner to just do, but maybe less efficient.
} else {
if(isDarkMode === 'true'){
darkModeOn()
} else if(isDarkMode === 'false'){
darkModeOff
} else {
darkModeOn()
localStorage.setItem('darkmode', 'true')
}
function toggleTheme(e) {
const isDarkMode = localStorage.getItem('darkmode')
if (isDarkMode === 'true') {
localStorage.setItem('darkmode', 'false')
darkModeOff()
} else if (isDarkMode === 'false') {
localStorage.setItem('darkmode', 'true')
darkModeOn()
}
}
@chrisdel101 yes I was referring to that. You can read more here: |
- clean up comments in theme js - wrap system theme check in function
Okay so I've add support for this. I looked for this feature before starting this task. I'd never heard of this before, but some other contrib comments talked about it. Since I didn't find anything I thought it was pseudo code. Doh! You this added feature at the top of More Explantion (reading optional)So I did not add use An overview of what's happening when system setting != dark: Current normal workflow (for user without system
|
Just fixed merge conflicts that appeared |
Thanks @chrisdel101, nice work .... One small comment: There are white bands at the top and bottom of each menu. I realize that this is also the case in current/bright mode, but it is much more noticeable in dark mode. Would it be a lot of work to eliminate them? If they're eliminated from regular/bright mode, so much the better... If it's too complicated or has other side-effects, we may be able to live with it. I'd also like to see what others think of the change in this PR, e.g. @jonchurch ... |
- remove bands on dropdowns - slight fix to first "installing" drop down
- rejig JS - add listener to mediaQuery - make latest change be the override
- rejig JS - add listener to mediaQuery - make latest change be the override
I made the fixes, and a few others I found in the process. They are all described in detail below. FIX 1There will hopefully be no side-effects since the rule here is quite specific, hitting just those links FIX 2After the above fixes I made this change: I think these look better without the border-radius, so the corners are now squared. This can easily be reverted if you don't agree FIX 3This is more of an FYI. |
I also changed the JS. Now the system setting JS media query has a listener so it does not take a page reload to take effect. screen-recording-2024-04-27-at-64731-pm_Do4mVu5o.mp4 |
- fix forgotten merge conflict line in header uz & ru
Thanks @chrisdel101 I know this was a lot of work. |
Added darkmode theme to site by clicking the moon icon. All pages available through the site drop downs have been tested for English, and a quick run through was done on chrome, FF, and Safari. Mobile testing was done with emulator and not on actual mobile device.
Support for all foreign languages also appears to be working. All foreign index pages tested.
Issues:
hello-world.html
cannot be styled, or at least I was not able to find a way. This section of the page remains white :(