Initial dark mode state - incorrect dark mode switching in AppShell example

drewbitt commented 1 year ago

What package has an issue

@svelteuidev/core

A clear and concise description of what the bug is

I am basing this off of the example AppShell/Header/HeadContent starting here

https://github.com/svelteuidev/svelteui/blob/bfbd3c221b292fd9591abb1976f4e8c2ef64933d/packages/svelteui-demos/src/demos/core/AppShell/AppShell.demo.usage.svelte#L43

isDark determines whether or not dark mode is enabled or not.

Set isDark to equal true at first instead of being false at first.

Now when dark mode is set initially with isDark

Light mode is shown first for immediately flashing to dark mode (even with prepareStylesSSR)
Switching from dark mode which was set as default to light mode causes the Header - the svelteui generated nav class - to have a dark theme while the rest of the content correctly switches to light

Whenever dark mode is set initially this occurs. This is a one line change in the demo code, which a lot of users might do when exploring SvelteUI and will find that this doesn't work

In which browser(s) did the problem occur?

No response

Steps To Reproduce

Use svelteui demo code Change let isDark = false to let isDark = true

Do you know how to fix the issue

No

Are you willing to participate in fixing this issue and create a pull request with the fix

Yes

Relevant Assets

No response

drewbitt commented 1 year ago

If you can't repro by changing the one isDark line, let me know and I can create a repro in a separate sveltekit project. This is in my own sveltekit app right now that uses svelteui demo code (SvelteUIProvider, Appshell, and Header with all their overrides that the demo code includes).

longnguyen2004 commented 1 year ago

Seems to be caused by the dark-theme class being used at 2 places, in the SvelteUIProvider div wrapper, which is not reactive to themeObserver, and in the root HTML element, which is reactive. Since the one in the div will override the one in the root element, this results in dark mode being stuck on if it's initially enabled.

longnguyen2004 commented 1 year ago

I was about to try to fix the issue, but I couldn't get the dev environment working. I cloned the repo, ran yarn install in the root dir, cd to packages/svelteui-core, ran yarn dev there, and got this error

failed to load config from D:\1. Coding\1.2. Open Source Contrib\svelteui\packages\svelteui-core\vite.config.js
error when starting dev server:
file:///D:/1.%20Coding/1.2.%20Open%20Source%20Contrib/svelteui/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:6
import { isCSSRequest } from 'vite';
         ^^^^^^^^^^^^
SyntaxError: The requested module 'vite' does not provide an export named 'isCSSRequest'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:189:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async loadConfigFromBundledFile (file:///D:/1.%20Coding/1.2.%20Open%20Source%20Contrib/svelteui/node_modules/vite/dist/node/chunks/dep-c842e491.js:64010:21)
    at async loadConfigFromFile (file:///D:/1.%20Coding/1.2.%20Open%20Source%20Contrib/svelteui/node_modules/vite/dist/node/chunks/dep-c842e491.js:63895:28)
    at async resolveConfig (file:///D:/1.%20Coding/1.2.%20Open%20Source%20Contrib/svelteui/node_modules/vite/dist/node/chunks/dep-c842e491.js:63519:28)
    at async createServer (file:///D:/1.%20Coding/1.2.%20Open%20Source%20Contrib/svelteui/node_modules/vite/dist/node/chunks/dep-c842e491.js:62819:20)
    at async CAC.<anonymous> (file:///D:/1.%20Coding/1.2.%20Open%20Source%20Contrib/svelteui/node_modules/vite/dist/node/cli.js:707:24)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Did I miss a step somewhere?

BeeMargarida commented 1 year ago

You need to run always in root, so if you do yarn dev in the root of the repo, it runs the storybook, which you can use to debug components

longnguyen2004 commented 1 year ago

The quickest workaround to the inconsistent theme problem is to make currentTheme reactive, like so

        const useStyles = createStyles(() => ({ root: {} }));
        const forwardEvents = createEventForwarder(get_current_component());
        const DEFAULT_THEME = useSvelteUITheme();
-       const currentTheme = () => {
+       $: currentTheme = () => {
                if (themeObserver === null) return null;
                return themeObserver === 'light' ? (mergedTheme as unknown as string) : (dark as string);
        };

That way, both the class in the root element and the wrapper div will change simultaneously, solving the inconsistency problem. However, it won't solve the problem of the background being briefly white. This is because the body's background is set using a .dark-theme body selector, but the dark-theme class isn't added to the root element until JS finishes executing. This is probably not something we can fix, since Svelte doesn't allow injecting classes into the root element during SSR.

I've found 4 ways to solve this, which are:

Use the <svelte:head> workaround
Use a server hook
Apply the background to the wrapper div instead of the body
Remove the background altogether and let the user handles it

longnguyen2004 commented 1 year ago

Also, can we get rid of the colorScheme store, and use the themeObserver value directly somehow? Having 2 separate variables that need to be kept in sync will probably break something in the future, so it might be worth looking into.

BeeMargarida commented 1 year ago

I'll need to take a closer look at that code for sure, I'll see if this week I can take some time to look at it properly.

BeeMargarida commented 1 year ago

Hey, I can't seem to replicate this in the latest version. Can anyone provide an example of this problem happening?

BeeMargarida commented 1 year ago

Reopening since it's not completely fixed

svelteuidev / svelteui