Submitted over 2 years ago

github user search - react , sass theming

Chamu• 12,970

@ChamuMutezva

Preview site View code

This is a solution for...

GitHub user search app

HTML
CSS
JS
API

2junior

View challenge

Design comparison

SolutionDesign

Solution retrospective

Another exciting challenge, let me know what you think i can improve. I used react for the challenge. I wanted the card to be a component on it's own getting its data through props from the the main parent. The data comes from an api and saved it using the state hook - the value of the state is the one that was received by the card component. But for some reasons that i still need to investigate the received data in the card was always undefined. I guess it is more to do with promises but if anyone can assist me to get around the issue .

Community feedback

Raymart Pamplona• 16,090

@pikapikamart

Posted over 2 years ago

This is the code in there.

const date = new Date(user.created_at)
const dateJoined = date.getDate()
const year = date.getFullYear()
const month = months[date.getMonth()]
console.log(year)
console.log(month)
console.log(dateJoined)

Basically, you are getting undefined and NaN is because you are accessing the user.created_at from the useState value and since the useEffect that holds the setState hasn't fired yet, the data will not be stored immediately. That is why at first run, you will get those results of undefined and NaN

So the best answer to prevent this is that, have a separate component that holds the whole card. One is your Main component, the other one is Card component. Then on your Main component, you set all the datas in there, then only use:

const Main = props =>{
  const [ user, setUser ] = useState({});
  .....
 insert useEffect to fetch data, convert it to async/await
  ........
   .....

 if ( user ) {  # only run this when user has the required Datas
  return <Card user={user} />
  }
}

This way, the Card component will only run if the user has loaded. The Card component will all have the data usage and you won't get any falsy values.

Marked as helpful

Chamu• 12,970

@ChamuMutezva

Posted over 2 years ago

@pikamart, 🤣🤣. The condition is what I was missing. Thank you

Raymart Pamplona• 16,090

@pikapikamart

Posted over 2 years ago

@ChamuMutezva Oh hahaha I was just about to suggest using context since I only thought there was only 1 fetch being made, I forgot that there is a searchbar hahaha

Well awesome if you made it work!

tediko• 6,560

@tediko

Posted over 2 years ago

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 use useRef() 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 in header.js component. You used document.querySelector for three elements which is completely unnecessary. As you know React provides a way to inject javascript right into our HTML using something called JSX 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. Now themeControl() look like this:

 const themeControl = () => {
    theme
      ? document.body.classList.add("theme-dark")
      : document.body.classList.remove("theme-dark");
  };

Now you can remove themeControl() call from handleClick since you want this to perform everytime your theme change. For this purpose in useEffect with themeControl function, add theme to dependency array. The dependency array in useEffect lets you specify the conditions to trigger it. So in our case we'll watch for every theme change and trigger useEffect only when that value change, like so:

useEffect(() => {
    themeControl();
}, [theme]);

I made a small refactor of your localStorage save option in header.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 to localStorage and one for retrieving that data.

Let's start with postThemeToLocalStorage function where we will set our newTheme value to globalTheme key. Note we don't need to include the window property. You can access built-in properties of the window object without having to type window. 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 our globalTheme key value from localStorage. You don't need to use JSON.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 and savedTheme variables. Then, in handleClick function, instead of manually save our data to localStorage, use postThemeToLocalStorage(!theme). Then, add useEffect 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 assign theme to be false (light theme) - const [theme, setTheme] = useState(false). Previously, when your localStorage was empty you got null 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! 💪

Marked as helpful

Chamu• 12,970

@ChamuMutezva

Posted over 2 years ago

@tediko , thanks so much for the awesome clear feedback. Will be implementing them before end of day. prefers-color-scheme was a bonus addition that i still have to do - it looks straight forward but will have to go over it again.

good point on the interpolation, indeed a lot of clean up and the javascript hang over is still there

will let you know how it goes

Raymart Pamplona• 16,090

@pikapikamart

Posted over 2 years ago

Let me just fork it and see if I can help with it

Chamu• 12,970

@ChamuMutezva

Posted over 2 years ago

@pikamart, ... The octocat is to be loaded on load, so my idea was to fetch its data in the main.js, then the card component was supposed to receive this octocat data as props.

Only the search was supposed to be done in the card component

Clément Creusat• 1,675

@ccreusat

Posted over 2 years ago

Woo !! It's really really nice . I would make animations a bit faster. Awesome job. Especially with the hover dashed borders for accessibility :)

AK• 6,700

@skyv26

Posted over 2 years ago

Wow! So nice and very close to original design. Wonderful work

Please log in to post a comment

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord