PS complete - Peter #4
Conversation
There was a problem hiding this comment.
Hey @PeterTheDeveloper - @ROgbonna1 asked me to take a look at your PR and leave a review. Overall nice work on this! I left some comment inline - biggest thing to change would be to use a prop for the UserCard instead of hard-coding it to a single userId, but overall this was well put together and your getUsers function was really easy to read. Also, the stying looked pretty cool from Materialize - nice work putting that together too! If you get a chance, see if you can use a prop to make your UserCard component more re-usable. Feel free to respond to any of the comments if you have questions!
| @@ -0,0 +1,62 @@ | |||
| import React from 'react'; | |||
| import { makeStyles } from '@material-ui/core/styles'; | |||
There was a problem hiding this comment.
Nice work applying the material UI styles - the card looks pretty nice!
| const [isLoading, setIsLoading] = React.useState(true); | ||
|
|
||
| React.useEffect(() => { | ||
| getUsers('3') |
There was a problem hiding this comment.
Nice work building out the getUsers function. One small note is that since we're only fetching one user, I'd probably name the function getUser instead - just a small change, but it helps let other developers know what to expect.
There was a problem hiding this comment.
One other thing here - right now, this component is only capable of rendering out the user with the ID of '3', but it could be capable of doing way more. Instead of hard-coding this value here, I'd have the component take in a prop called userId that you could use to fetch the data. That way, you could render out this component a bunch of different times.
| @@ -0,0 +1,19 @@ | |||
| import { UserCard } from "./UserCard"; | |||
There was a problem hiding this comment.
You shouldn't need to import this here - this function will get used in the UserCard Theoretically, this function could be used in other spots of your app too, it's really flexible!
|
|
||
| async function getUsers(userID) { | ||
|
|
||
| const url = `https://reqres.in/api/users/${userID}`; |
There was a problem hiding this comment.
👍 👍 - nice work giving this variable a name! Small thing, but it makes this way easier to read.
| const results = JSON.parse(data); | ||
| return results.data; | ||
| } | ||
| catch (err) { |
There was a problem hiding this comment.
Also 👍 👍 to catching this - you could alsocatch in you component to display some type of error there if you wanted to as well.
| setUser(data); | ||
| setIsLoading(false); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
If you did update this to use a userId prop, you'd also want to include that here in the dependency array so that the useEffect callback could fire if that changed.
| }); | ||
|
|
||
| const UserCard = () => { | ||
| const [user, setUser] = React.useState({ |
There was a problem hiding this comment.
This is kind of cool - when I did this, I used null as the default value for the user, but having the keys here lets you know what to expect to be included, which is nice. I'd probably use null for the property values instead of loading..., as the loading message will display kind of weird for things like the avatar.
{
avatar: null,
email: null,
//...etc
}| }, []); | ||
|
|
||
| const classes = useStyles(); | ||
|
|
There was a problem hiding this comment.
Instead of depending on your default '...loading' values in state, you can use the isLoading prop to do an early return for the loading message.
if (isLoading){
return <p>...Loading</p>
}I think that makes things a bit easier to reason about.
No description provided.