Problem
I am a student who wrote a theme switcher, but I don’t know if everything is ok.
I want professionals to review my code and point out mistakes. I accept all recommendations and criticism.
src/components/ThemeSwitcherApp/
import React, {useEffect, useState, useMemo, useCallback} from "react";
import DarkStyles from "../DarkStyles/DarkStyles";
export const ThemeSwitcherApp = () => {
const key = "DARK_THEME";
const saveJSON = (key, data) => {
if (window[`localStorage`] === null) return
return localStorage.setItem(key, JSON.stringify(data));
};
const loadJSON = key => {
if (window[`localStorage`] === null) return
return key && JSON.parse(localStorage.getItem(key));
};
const [themeIsActive, setThemeIsActive] = useState(loadJSON(key) ?
loadJSON(key).darkMode : false);
const [button_text_value, setButton_text_value] = useState(themeIsActive ? "Turn off" : "Turn on");
const theme_data_false = useMemo(() => ({darkMode: false}), []);
const theme_data_true = useMemo(() => ({darkMode: true}), []);
const insertDarkStyles = (where) => {
const style = document.createElement(`style`);
style.setAttribute(`id`, `dark-mode`);
style.innerHTML = DarkStyles;
where.append(style);
};
const ActiveDarkTheme = useCallback(
() => {
insertDarkStyles(document.getElementsByTagName(`head`)[0]);
setButton_text_value("Turn off");
saveJSON(key, theme_data_true);
}, [theme_data_true]
)
const DeactivateDarkTheme = useCallback(
() => {
const isDarkModeExist = !!document.getElementById(`dark-mode`);
if (isDarkModeExist) {
document.getElementById(`dark-mode`).remove();
setButton_text_value("Turn on");
saveJSON(key, theme_data_false);
}
}, [theme_data_false]
)
const ThemeStorageLoading = useCallback(
(key, data) => {
if (!localStorage.getItem(key)) {
saveJSON(key, data);
} else {
if (loadJSON(key).darkMode) {
ActiveDarkTheme();
} else {
DeactivateDarkTheme();
}
}
}, [ActiveDarkTheme, DeactivateDarkTheme]);
useEffect(() => { // Once Loading
ThemeStorageLoading(key, theme_data_false);
}, [theme_data_false, ThemeStorageLoading]);
const ButtonAction = (bool_value) => {
setThemeIsActive(bool_value);
if (bool_value) ActiveDarkTheme();
else DeactivateDarkTheme();
};
return (
<div>
<input type="button"
onClick={() => ButtonAction(!themeIsActive)}
defaultValue={button_text_value}
style={{cursor: "pointer"}}
/>
</div>
);
}
```
Solution
- variable
key
can relate to anything. I’d just call this variabletheme
- I’d also avoid naming the function parameter
where
- If You are using an arrow function and it has one parameter then You can avoid adding
()
around it - Why can’t You just use
button
instead of<input type="button">
? style={{cursor: "pointer"}}
I’d remove that from the JSX and place that in the css document- Sometimes you name things with
camelCase
and sometimes You usesuch_a_thing
. Try to stay consistent - Function names inside Your component should start with lowercase
- It’s a good practice to use
{}
to wrap the blocks of code in Yourif/else
statements - Additionally, I’d change the approach a little bit. If I had the task to do the theme switching functionality, I’d create the toggle which will change the
isDarkMode
flag totrue/false
. If the flag is true, then I would add thedark-mode-active
on the body element of the page. After that I would create the proper CSS file and put the dark mode related styles to the class we’ve just attached to the body element. Once loading
this comment is not necessary. Use comments to describe some workarounds / hacks / code tricks but such a fundamental functionality likeuseEffect
does not require any comments 🙂
I hope that You will find the above feedback useful. Good job with the above and good luck with the improvements