- HTML
- CSS
- JS
- API
github user search - react , sass theming
9
Hello, Chamu! 👋
Congrats on finishing another challenge! 🎉 Your solution looks very good and also responds well. Here's some feedback:
- You left a lot of
console.logmethods in your code. Get rid of this as it makes the code harder to read. - A common convention which is widely followed is that component should follow PascalCase naming convention. The name of the folder/filename will always reflect what is present in that file that's why it is good to also use PascalCase for them, not only function within that file. e.g. (Header/Header.js)
- You should avoid using
document.querySelectorwith React. There is only few cases (sometimes there is still a need for manipulating elements directly) where it can be useful but in most cases there is better way to approach that. Instead you can useuseRef()hook. Refs are more reliable for getting the specific DOM node you care about.document.querySelectorcan give unexpected results if there are multiple elements that match the query and if you're writing a component that will show up multiple times in many places this problem will occur, that's why refs become more of a clear-cut winner. - Take a look at
themeControlfunction inheader.jscomponent. You useddocument.querySelectorfor three elements which is completely unnecessary. As you know React provides a way to inject javascript right into our HTML using something calledJSX interpolation. So instead of taking element from DOM with querySelector and then inject some data with.innerHTMLor.srcmethods you should simply create a expression inside your HTML elemetns like so:
/* If theme is true return "Dark" if not return "Light". */
<span className="mode__state">{theme ? "Dark" : "Light"}</span>
/* If theme is true return Moon element if not return Sun element. */
<img className="mode__img" src={theme ? Moon : Sun} alt="" />
- To completely get rid of querySelector from this function we can use
document.bodyproperty. It represents the<body>node of the current document. Along with this, I used conditional (ternary) operator to either add or remove theme-dark class. NowthemeControl()look like this:
const themeControl = () => {
theme
? document.body.classList.add("theme-dark")
: document.body.classList.remove("theme-dark");
};
- Now you can remove
themeControl()call fromhandleClicksince you want this to perform everytime yourthemechange. For this purpose inuseEffectwiththemeControlfunction, addthemeto dependency array. The dependency array in useEffect lets you specify the conditions to trigger it. So in our case we'll watch for everythemechange and trigger useEffect only when that value change, like so:
useEffect(() => {
themeControl();
}, [theme]);
- I made a small refactor of your
localStoragesave option inheader.js. In functional programming it is good to break up your code into small reusable functions. For this, we will create two functions, one to save data tolocalStorageand one for retrieving that data.
- Let's start with
postThemeToLocalStoragefunction where we will set ournewThemevalue toglobalThemekey. Note we don't need to include thewindowproperty. You can access built-in properties of the window object without having to typewindow.prefix. The window object is the top-level object of the object hierarchy. As such, whenever an object method or property is referenced in a script without the object name and dot prefix it is assumed by JavaScript to be a member of the window object. So our function will look like this:
const postThemeToLocalStorage = (newTheme) => {
localStorage.setItem("globalTheme", newTheme);
};
- Then let's create
getThemeFromLocalStoragefunction where we want to return ourglobalThemekey value fromlocalStorage. You don't need to useJSON.parse()method since our value from localStorage is a string, not an object. As the localStorage value cannot be saved as boolean we want to simply convert it. For this, we make a simple comparison, to return either true or false but as boolean values, like so:
const getThemeFromLocalStorage = () => {
return localStorage.getItem("globalTheme") === "true";
};
- Now, we have to clean up the code a little since we have a few unnecessary things here. Remove
localStorageandsavedThemevariables. Then, inhandleClickfunction, instead of manually save our data to localStorage, usepostThemeToLocalStorage(!theme). Then, adduseEffecthook, that will only trigger on render (by providing empty dependency array) and save our theme from localStorage to theme state, like so:
useEffect(() => {
setTheme(getThemeFromLocalStorage());
}, []);
- Finally, we have to assign a starting value for our
themestate. Since you don't use anything like prefers-color-scheme to detect if the user has requested a light or dark color theme, we assignthemeto befalse(light theme) -const [theme, setTheme] = useState(false). Previously, when your localStorage was empty you gotnullvalue from storage.nullis a falsy value so it still works, but we want to avoid such cases.
I'll send you my copy of header.js on slack just if I forgot to include here something. Good luck with that, have fun coding! 💪
1
