-
Notifications
You must be signed in to change notification settings - Fork 302
Add dark mode scss #1343
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 scss #1343
Conversation
cc @Turbo87 for a review (unsure who is actually in charge for the blog) thanks |
badcba7
to
1974154
Compare
@GuillaumeGomez I think we can now review the CSS for the dark mode. Took me a while to figure out a workflow allowing me to test all the changes ensuring that nothing in the light mode would change. I like the colors of the dark mode (see screenshots in the first comment) but I'm open to changes. HTML var names are also a bit random, I'm happy to receive suggestions for more meaningful names. In a follow-up patch, I'd like to add a button to switch theme. This will unfortunately make the blog not javascript-free anymore, I'm afraid 😞 Let me know your thoughts! Thanks :3 |
Apart from the two variables with confusing names, it looks all good to me. Great job!
For me, the important part isn't that it's actually JS-free but rather than it works without JS and that JS only provides limited and secondary improvements. So handling themes with a button doesn't seem that bad to me (and will not require a lot of JS either). |
ok @GuillaumeGomez I've added a theme selector 🙂 Notes:
|
Looks all good to me, thanks! Now the big question is: who is allowed to merge in this repository? I can but not sure I should. 😅 |
ok @GuillaumeGomez: as far as I can see - we should be there. I can't think of anything else to add right now. (I'll update the screenshots in the opening comment to reflect the latest version) |
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.
Looks good to me. Some non-blocking thoughts:
- Is it possible to switch to a white version of the Rust logo? The black one is hard to see on the dark background.
- Are there actually people who have dark mode enabled and go around manually reenabling light mode on certain websites? The javascript seems unnecessary to me, but I might be wrong.
- The formatting changes are a bit irritating, but setting up auto-formatting and CI is something for another PR.
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.
Can be merged with Remo's comments addressed
b5fdd8f
to
069e913
Compare
5f3edec
to
6822157
Compare
hey @senekor I've fixed the Rust logo and removed the formatting to make it a clean PR. About the JavaScript to switch theme, I kind of agree with you, I was hesitant either but every Rust site has that (rustdoc, www, not to mention clippy's that without JS is broken) 🤷♂️ If you feel strongly about it, happy1 to remove it. Footnotes
|
FWIW I was very confused when I visited the blog and it defaulted to dark mode. I double checked by system setting and it was in light mode, but changing it back and forth didn't help. It took a while until I discovered the theme switcher dropdown. is there a reason why we don't default to the system setting? |
hmm weird, I guess I must have had an old override in my localstorage somehow. I would still prefer it to follow my system setting though 😅 |
We could always add a "system value" option like there is in rustdoc. |
The implementation should be:
You should be able to verify with a clean browser profile. |
PR to add system setting: #1448 |
First attempt at avoiding being shock blinded when visiting the blog 😄
Note: this CSS applies only to the blog i.e. external links to www.rust-lang.org will take users to
#fff
land for now.Preview (top)
Preview (bottom)
Example blog post
Mobile view (simulated)
credits to @teohhanhui for providing 80% of this work, thanks!