[fix](@svelteui/core): fix `dark-theme` class inconsistency

svelteuidev / svelteui

SvelteUI Monorepo

https://svelteui.dev

MIT License

1.28k stars 64 forks source link

[fix](@svelteui/core): fix `dark-theme` class inconsistency #368

Closed longnguyen2004 closed 1 year ago

longnguyen2004 commented 1 year ago

Description

Making currentTheme reactive, so the wrapper div class syncs up with the html class. Partially fixes #334

Before submitting the PR, please make sure you do the following

[x] Read the Contributing guide
[x] Prefix your PR title with [@svelteui/core], [@svelteui/actions], [@svelteui/motion], [@svelteui/core], [core], or [docs].
[x] Include a description of the changes made in the PR description and in the commit messages.
[ ] Include screenshots/videos of the changes made.
[x] Verify that the linter and tests are passing, with yarn lint and yarn test or just run yarn prepush.

BeeMargarida commented 1 year ago

Hi! Thank you for this contribution! I'll not immediately merge since I want to analyze the issue again, since I'm not 100% sure what the cause yet.

BeeMargarida commented 1 year ago

Sorry, been busy with work, will check this out this week!

BeeMargarida commented 1 year ago

I can't seem to replicate the issue that this PR seems to fix. Can you?

longnguyen2004 commented 1 year ago

Here's a StackBlitz repro of the issue https://stackblitz.com/edit/svelteui-dark-mode-bug-repro And here's a video describing the bug

https://github.com/svelteuidev/svelteui/assets/15729831/4a68e257-de3f-4328-857f-0dc390663d5c

Basically, if the initial theme state is dark, then some components will be stuck in the dark state, because currentTheme is not reactive, so the dark-theme class is stuck on in the wrapper div.

There's also the issue of the background being initially white, before switching to dark, which is due to this not being SSR friendly. I didn't include a fix for that here, but I've outlined potential fixes for it in https://github.com/svelteuidev/svelteui/issues/334#issuecomment-1510995639

BeeMargarida commented 1 year ago

After merging this I'll make a new release, and fixing this for good will be the focus of the next one, I think

longnguyen2004 commented 1 year ago

While you're at it, have a look at #373 and #370 too, those are trivial fixes

BeeMargarida commented 1 year ago

While you're at it, have a look at #373 and #370 too, those are trivial fixes

Yap, looking at them now. I'll see if I can fix them and make a release this afternoon