Add dark/light mode switch buttons in Options menu #1950
Conversation
|
@Adoliin, thanks for the code! Would be awesome to get this in at some point. FWIW, I've also tested this together with fava-dashboards. It needs some changes to the library like Evernight/fava-dashboards@9872223 but works well after that. Didn't get a chance to try #1883 before. Personally, I think a tidy single icon (not a toggle) next to the filters could be well-fitting as well but switch in the options works nicely too (imo). Not qualified to comment on CSS changes though. |
|
@Evernight, Thanks, just let me know what issue this PR is causing with fava-dashboards |
|
@yagebu, I just wanted to follow up on this PR. It's been a month since submission, and I wanted to check if there's anything I can do to help move it forward. Please let me know if any changes are needed. Thanks :) |
@Adoliin , I already have a fix ready, just in case: andreasgerstmayr/fava-dashboards@main...Evernight:fava-dashboards:fava-theme-switch It's basically replacing the selector in css, identical to your changes, and also adjusting the JS logic a little bit. By the way, I have patched versions of Fava and dashboards with the switch running together in more recent versions of https://github.com/Evernight/lazy-beancount since I really needed the ability to switch between themes. |
There was a problem hiding this comment.
Thanks for the PR!
I wasn't really happy with the need for duplicating all the styles, so I wanted to research some alternatives. I think a better way is possible - in #2026 I converted the light/dark-mode styles to use the light-dark() function. So now, it should be possible to override the color scheme by simply overriding the color-scheme CSS attribute. Can you rebase your changes on main and make that adjustment? I've also left some other comments.
| <h2> | ||
| {_("Theme style")} | ||
| </h2> | ||
| <ThemeToggle /> |
There was a problem hiding this comment.
Can you reuse the ModeSwitch.svelte component instead of creating a new component for this?
| return window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; | ||
| } | ||
|
|
||
| export const theme = writable<string>(getInitialTheme()); |
There was a problem hiding this comment.
For having stores synced to localStorage, there's already localStorageSyncedStore, can you use that?
| export const theme = writable<string>(getInitialTheme()); | ||
|
|
||
| // Subscribe to theme changes | ||
| theme.subscribe((value) => { |
There was a problem hiding this comment.
Since this is startup code that attaches subscribers, it should not be on the top level but wrapped in a function that is called from main.ts
| @@ -135,6 +135,14 @@ function init(): void { | |||
| }); | |||
|
|
|||
| router.trigger("page-loaded"); | |||
|
|
|||
|
|
|||
| const storedTheme = localStorage.getItem("theme") ?? "auto"; | |||
There was a problem hiding this comment.
I don't think this logic here is needed, adding / removing the attribute should all happen in the store.subscribe (but actually attaching that subscriber should happen here)
Solve these 2 issues:
#1883
#1874
Descriptiopn
The website style can now be manually set by going to
Optionsin the side bar menu and selecting one of the 3 optionsSystemLightDarkScreenshot