fix: store dark/light theme mode in localStorage

[echelonnought]

Description

Persisted themes to localStorage

screenshots

Closes #383

[shootermv]

i wonder -
if we change return getItem() === 'true' || false
to just return getItem()
will it still work?

[shootermv]

or even simplier: const [isDarkTheme, setIsDarkTheme] = useState(getItem())

[echelonnought]

Ahh yeah, that's correct it can be simplified as such

[shootermv]

one more thought i have now: do we have to convert the isDarkTheme value to string before storing it at localStorage?

[echelonnought]

that is an interesting question @shootermv, I was converting that to string because i think local storage stores both it's values and keys as strings, so i wanted to maintain that flow of data. i could be wrong though. what do you think?

[NoamGaash]

I think it can be removed

[shootermv]

it can be removed but you also should change const [isDarkTheme, setIsDarkTheme] = useState(getItem())
to
const [isDarkTheme, setIsDarkTheme] = useState(getItem() === 'true')
since getItem() returns string not boolean

[echelonnought]

Regardless, we can still set the getItem() in setIsDarkTheme within the useEffect hook and it would still work fine @NoamGaash and @shootermv.
Like so:

useEffect(() => {
   const storedTheme = getItem()
   setIsDarkTheme(storedTheme)
 }, [])

[NoamGaash]

use useMemo, not useEffect+state
https://stackoverflow.com/questions/56028913/usememo-vs-useeffect-usestate

[NoamGaash]

you know what? why would you need it to be a state variable?
you can simply use:

    const isDarkTheme = getItem() === 'true'

[echelonnought]

Apologies for chiming in this late @NoamGaash. Had issues with power where I'm at.
I am reading up the resource you shared from stack overflow, and I have some questions though;
Is it proper to not use useEffect when dealing or working with external web apis like local storage which are necessary side effects or useMemo handles this too properly?
If useMemo does that, then it'd be a good option I think.

[NoamGaash]

@echelonnought I think each case should be considered individually, but generally speaking -

1. It's good to avoid unnecessary renders. Every setState makes the component rerender, so a) it's not very elegant and b) the first render has the "wrong" state value
2. When one state variable based on information from another variable, most probably that it shouldn't be a state variable
3. I prefer using useMemo in case that the variable is a literal object/array (it means it will have different pointer each render). in your case, it's a primitive value, so you don't even need the memoization part.

That's why I'm in favor using const isDarkTheme = getItem() === 'true', and not const isDarkTheme = useMemo(getItem() === 'true', [getItem()]) or something similar.

That's my opinion, hope it answered your question

[echelonnought]

Yeah those are pretty solid reasons. especially the useMemo part, if i had my way, i would use useRef to avoid rerenders, i tried but ran into some issues, i was storing those values in local storage but it didn't change with the toggle button, however since we are changing the UI through the theme change, I find it appropriate to use useState instead, to handle that whenever the UI changes. but I pretty much agree with you on every point you listed!

I am replying late because i've had power issues and trying to catch up with work too.

I'll join the slack and discord to follow with the conversations on this!
Thank you, @NoamGaash !

[shootermv]

@echelonnought do you need a help with solving a confilcts?

[echelonnought]

@echelonnought do you need a help with solving a confilcts?

Yeah I am very much interested in resolving these conflicts, had power outage.

[shootermv]

Did you joined our discord chanel?

…

On Thu, Jan 25, 2024, 02:17 Ngole Lawson ***@***.***> wrote: @echelonnought <https://github.com/echelonnought> do you need a help with solving a confilcts? Yeah I am very much interested in resolving these conflicts, had power outage. — Reply to this email directly, view it on GitHub <#418 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKGMHUEDVPSZQB2DT4UAM3YQGQDJAVCNFSM6AAAAABCDYLC46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBZGEZTONBSGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[NoamGaash]

@shootermv I think he's an English speaker
Yet, most of us speak english fluently, so please feel free to chat with us
slack:
https://join.slack.com/t/hasadna/shared_invite/zt-21qipktl1-7yF4FYJVxAqXl0wE4DlMKQ
discord:
https://discord.gg/f5UxKQrg

[shootermv]

for merging the latest changes all you need - is to run:
git fetch upstream
git merge upstream/main
and may be, resolve the conflicts
feel free to connect at me (or someone else) discord if you need help

[NoamGaash]

Hi @echelonnought , I made some suggestions. Let me know what you're thinking

[echelonnought]

Hi @echelonnought , I made some suggestions. Let me know what you're thinking

Saw the types enforced in the useLocalStorage file, also noticed that you took out the useEffect hook. Thanks on that, care more to explain on the latter decision please @NoamGaash, I mean wouldn't it hurt performance ?

[NoamGaash]

Hi @echelonnought !
Omitting the useEffect & useState should improve performance, as re-renders will no longer be necessary.
Regarding to type definitions - it shouldn't have performance impact, because typescript transpile to javascript during the build process, and type information is omitted during the transpilation.
Hope it answer your question :)
BTW, have you seen the test? 😃
Now we can be sure that no-one will accidently break it.

[NoamGaash]

My bad, I forgot to push the latest commit 😅
#451