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

dark mode option #90

Closed
wants to merge 2 commits into from
Closed

dark mode option #90

wants to merge 2 commits into from

Conversation

snoyer
Copy link
Contributor

@snoyer snoyer commented Apr 13, 2024

Proposal to add "dark mode" capability by overriding the colors in CSS.
The overriding can be ignored (by default, same as current behavior), forced/hardcoded, or conditional to match the client's @media (prefers-color-scheme: dark) setting.

I'm not sure what the defaults should be or how the option needs to be handled to play nice in the web app. Also the proposed dark color values are auto-generated by flipping their HSL value, picking them by hand could achieve better results but I'm not competent for that.


The following rendering of examples/showcase.yaml with --dark-mode=auto should adapt to the client browser's or OS's preference:
showcase-auto

@snoyer
Copy link
Contributor Author

snoyer commented Apr 13, 2024

Screen capture of expected behavior when changing the OS theme which gets propagated to the browser and then the generated SVG:

keymap-drawer-dark-mode.webm

@caksoylar
Copy link
Owner

Thank you for the PR, honestly this is an issue that's been bugging me for some time since the current theme doesn't look great on dark backgrounds and I had also tried implementing an auto mode. It's good to see that it works well embedded on Github, since that's where most people view it. I also have eventual plans to introduce theming, in which case dark mode would be one of the themes. But your implementation seems pretty nice right now, and I don't want perfect to be the enemy of good.

Given that I think I will merge this, but I'll probably spend some time tweaking colors.

Re: web app, it has issues regarding detection of light/dark (e.g. streamlit/streamlit#6456) so the auto mode doesn't work. But I'll add this setting to the quick config boxes and probably default to auto for the app nevertheless.

@snoyer
Copy link
Contributor Author

snoyer commented Apr 13, 2024

Cool, happy to help even if this PR ends up being just a proof of concept :)
I just fixed a dumb bug when I forgot to remove the original CSS output line and the whole thing was duplicated.

Not sure if you want me to investigate pylint's complaints or if you want to refactor anyway?

@caksoylar
Copy link
Owner

Thanks again. Yeah, I'll probably end up refactoring it before merging, I'll handle the issues.

@caksoylar caksoylar closed this in 69f7092 Apr 14, 2024
@snoyer
Copy link
Contributor Author

snoyer commented Apr 14, 2024

nice! Your hand-picked colors do look better than my procedural ones. Just needs auto dark mode on the logo now ;)

@snoyer snoyer deleted the dark-mode branch April 14, 2024 07:52
@caksoylar
Copy link
Owner

Glad you like it. Replaced the logo as well now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants