fix: save user theme to settingsStore

[GregoMac1]

Closes #78
TODO: The theme is correctly stored but, if the saved theme is dark, when user reloads the page the initial theme is light and then it turns to dark (because it gets changed onMount).

[fmaclen]

when user reloads the page the initial theme is light and then it turns to dark (because it gets changed onMount)

I think the "correct" way of doing this is not to use onMount() and instead use: import { browser } from '$app/environment';

Here's an example (Source):

<script lang="ts">
    import { browser } from '$app/environment';

    let darkMode = true;

    function handleSwitchDarkMode() {
        darkMode = !darkMode;

        localStorage.setItem('theme', darkMode ? 'dark' : 'light');

        darkMode
            ? document.documentElement.classList.add('dark')
            : document.documentElement.classList.remove('dark');
    }

    if (browser) {
        if (
            localStorage.theme === 'dark' ||
            (!('theme' in localStorage) && window.matchMedia('(prefers-color-scheme: dark)').matches)
        ) {
            document.documentElement.classList.add('dark');
            darkMode = true;
        } else {
            document.documentElement.classList.remove('dark');
            darkMode = false;
        }
    }
</script>

In another project I had a similar implementation that used this technique:
https://github.com/search?q=repo%3Afmaclen%2Fcanutin-desktop%20colorThemeStore&type=code

But that alone won't solve the issue with the FOUC (Flash of Unstyled Content), there is one more piece to the puzzle in app.html where we read the localStorage outside of Svelte.

You can try it out in the live demo: https://demo.canutin.com/settings

[GregoMac1]

I tried these options and I came to a solution where the initial theme is selected in app.html. Then, +layout.svelte only handles the theme switch. This removes the FOUC and works correctly.

[fmaclen]

This looks good so far 👍

I'd like to get that initial theme change bug you pointed out too.
I left you some suggestions on how to fix that issue.

[cloudflare-workers-and-pages]

Deploying hollama with    Cloudflare Pages

Latest commit:	9dbb3f0
Status:	✅  Deploy successful!
Preview URL:	https://40796900.hollama.pages.dev
Branch Preview URL:	https://save-user-theme.hollama.pages.dev

View logs

[GregoMac1]

@fmaclen this has conflicts with the tests but the conflicts should be gone if the branches get merged. I tested it locally.

[fmaclen]

The functionality appears to be working fine now and the rest of the PR looks good 👍
Just left you a note about a potential code smell when updating the store.

[fmaclen]

This is not necessarily wrong, but one thing to note is that inside a Svelte component you don't need to call settingsStore.update(), you can simply do:

$settingsStore!.userTheme = window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light';

Assignments to $-prefixed variables require that the variable be a writable store, and will result in a call to the store's .set method. — https://svelte.dev/docs/svelte-components#script-4-prefix-stores-with-$-to-access-their-values

Also, I'm not sure what the best way to handle this is but if $settingsStore == null (which I don't know if it's even possible in this case), calling $settingsStore! would throw an error to the console, right? Which I guess it's useful because we are always expecting the store to be present not matter what.

In other places I added a conditional check, but maybe we actually need to intentionally throw an error if we really are expecting settingsStore to be set at that point.

I personally almost never use the syntax variable!, feels a bit like a code smell. My gut tells me that in both onMount() and toggleTheme() we'd be better off adding a guard clause, specially onMount() since running the rest of the code in that section doesn't make sense if the store isn't present for whatever reason.

What do you think?

[fmaclen]

Maybe something like this can make the code a bit more readable?

onMount(() => {
	if (!$settingsStore || !browser || theme) return;
	$settingsStore.userTheme = window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light';
});

function toggleTheme() {
	theme = theme === 'light' ? 'dark' : 'light';
	document.documentElement.setAttribute('data-color-theme', theme);
	if ($settingsStore) $settingsStore.userTheme = theme;
}

[GregoMac1]

Nice! I agree. I don't like using variable! either, but I also thought about the need of settingsStore to always be present.

I've just commited this, so I am merging now.

[fmaclen]

🎉 This PR is included in version 0.7.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀