Merge #8
Merge #8
Conversation
ipc103
left a comment
ipc103
left a comment
There was a problem hiding this comment.
Hey @Deteriator - @ROgbonna1 asked me to take a look at your PR. Nice work here! The styling is pretty cool, and selecting the user ID randomly was a cool idea. I left a few other comments inline - feel free to respond if you have any questions!
| first_name: 'loading...', | ||
| last_name:'loading...' | ||
| }); | ||
| let number = Math.floor(Math.random() * 12) + 1 |
There was a problem hiding this comment.
This is pretty cool! It might be neat to store the number in state, and then give the user a button to update it.
const randomUserId = () => Math.floor(Math.random() * 12) + 1 ;
const [userId, setUserId] = useState(randomUserId());
const handleClick = () => setUserId( randomUserId() );There was a problem hiding this comment.
Actually, now that I think about it more, I'd probably include this logic in the parent App component, and then pass the userId as a prop to this component. That makes the user card a little bit more flexible.
| <CardActionArea> | ||
| <CardMedia | ||
| component="img" | ||
| alt="Contemplative Reptile" |
There was a problem hiding this comment.
I'd probably use a more descriptive alt text here, maybe User Avatar or Profile Picture? This is helpful for screen readers to identify the content. https://accessibility.psu.edu/images/alttext/
| const [isLoading, setIsLoading] = React.useState(true); | ||
|
|
||
| React.useEffect(() => { | ||
| getUsers(number.toString()) |
There was a problem hiding this comment.
small comment, but you actually don't need to convert the number to a string here, since the interpolation will take care of that for you.
| @@ -0,0 +1,19 @@ | |||
| import { UserCard } from "./UserCard"; | |||
There was a problem hiding this comment.
You can remove this import statement, since this function doesn't actually use the UserCard.
|
|
||
| async function getUsers(userID) { | ||
|
|
||
| const url = `https://reqres.in/api/users/${userID}`; |
There was a problem hiding this comment.
This function is well put together - easy to read, and having the url variable is a nice touch. 👍 👍
| let number = Math.floor(Math.random() * 12) + 1 | ||
| const [isLoading, setIsLoading] = React.useState(true); | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
I tend to prefer to import my hooks from React in the import statement because I get tired to typing React. Some people prefer to just import the default and call this directly, so totally up to you!
|
|
||
| const UserCard = () => { | ||
| const [user, setUser] = React.useState({ | ||
| avatar:'loading...', |
There was a problem hiding this comment.
When the page first loads, the image appears as a broken link, because 'loading...' isn't a valid src. Given that you have the isLoading state below, I think I'd be more inclined to set all of these default values to null, and then conditionally render a loading message if the component isLoading
if(isLoading){
return <p>Loading...</p>;
}
// rest of the render2 participants
No description provided.