- HTML
- CSS
- JS
- API
github user search - react , sass theming
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.log
methods 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.querySelector
with 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.querySelector
can 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
themeControl
function inheader.js
component. You useddocument.querySelector
for 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.innerHTML
or.src
methods 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.body
property. 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 fromhandleClick
since you want this to perform everytime yourtheme
change. For this purpose inuseEffect
withthemeControl
function, addtheme
to dependency array. The dependency array in useEffect lets you specify the conditions to trigger it. So in our case we'll watch for everytheme
change and trigger useEffect only when that value change, like so:
useEffect(() => {
themeControl();
}, [theme]);
- I made a small refactor of your
localStorage
save 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 tolocalStorage
and one for retrieving that data.
- Let's start with
postThemeToLocalStorage
function where we will set ournewTheme
value toglobalTheme
key. Note we don't need to include thewindow
property. 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
getThemeFromLocalStorage
function where we want to return ourglobalTheme
key 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
localStorage
andsavedTheme
variables. Then, inhandleClick
function, instead of manually save our data to localStorage, usepostThemeToLocalStorage(!theme)
. Then, adduseEffect
hook, 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
theme
state. Since you don't use anything like prefers-color-scheme to detect if the user has requested a light or dark color theme, we assigntheme
to befalse
(light theme) -const [theme, setTheme] = useState(false)
. Previously, when your localStorage was empty you gotnull
value from storage.null
is 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! 💪