finished, didn't get to style the way i want

[paulsobers23]

No description provided.

[ipc103]

Hi @paulsobers23 - @ROgbonna1 asked me to take a look at your PR here and leave a few comments. Really nice work! I left a few comments in line, but generally this was really well organized and thought through. Nice job separating out the getUser function - that was a nice touch! Feel free to respond to any of the comments if you have further questions.

[ipc103]

This is just a small thing, but React will actually let you render out an array of elements. This means that you can use the map function to build out a group of components like this. You could imagine doing something like:

const userIds = [1, 2, 3, 4, 5, 6];

return (
  <div>{userIds.map((userId) => <UserCard userID={userId} /> )}</div>
);

The nice thing about that is, if you want to render additional cards, you can simply update the array with new values. You could even imagine pulling that list of IDs from an API or something later at some point, too.

[paulsobers23]

Oh wow, I did not know that will refactor to this, thank you !

[ipc103]

A couple of small notes here:

1. Since this piece of state is representing a single user, I think it makes more sense for the initial value to be null or an empty object instead of an array. Setting the initial value as an array signals to other developers to expect a collection of many things.
2. Along those same lines, I would name the function setUser instead of setUsers - it's a small change, but helps to communicate what the code is doing.

[paulsobers23]

Yea that makes a lot of sense, will also refactor this, thank you.

[ipc103]

👍 to the try / catch block here as well. You could also add a catch callback in your component to display an error to the user if something goes wrong.

getUser(userId)
  .then(user => setUser(user))
  .catch(error => {
    setError(err)
    setIsLoading(false)
  })

[paulsobers23]

I did not know that, that looks so more readable also, thank you

[ipc103]

One other small comment - this works well and is very readable to have the isLoading state. Since you're really only waiting on the user, if you set the initial value of the user to null, you could also use that to determine if you should be "Loading" or not. Something like

const [user, setUser] = useState(null);

// in the render

{ !user ? ("...Loading") : // render the rest of the component }

There's no right or wrong way to do it, just another idea that you can use. Feel free to choose whatever is more readable to you for your own projects!

[paulsobers23]

Yea I think this also reads more because we are waiting for the user to come back from the API and also would save some lines.