-
Notifications
You must be signed in to change notification settings - Fork 0
Add components #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for exquisite-pothos-d44e7c ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, really love the design with a masonry layout!
Haven't seen pagination, also useEffect
makes the CPU fly to the moon.
Overall, good job!
@@ -1,31 +1,30 @@ | |||
import { useState } from 'react' | |||
import { useState, useEffect } from 'react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not forget do delete unused code :)
|
||
useEffect(() => { | ||
getDetail(id).then(value => setPokemon(value)) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not forget to add dependency array as a second argument
useEffect(() => {
getDetail(id).then(value => setPokemon(value))
}, [id]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let getDetail = async function(id: string): Promise<Pokemon> { | ||
const ability = (await getOne(id)).data.abilities[0].ability.name; | ||
const numId = parseInt(id); | ||
const name = (await getList(numId - 1, 1)).data.results[0].name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to make second request for one pokemon from the list, one can obtain name
from the getOne
request too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (pokemon == undefined) { | ||
return ( | ||
<h1>Loading...</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like that you added loading placeholder, good job
|
||
useEffect(() => { | ||
getList(0, 9).then(values => setPosts(values.data.results)) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with dependencies array
useEffect(() => {
getList(0, 9).then(values => setPosts(values.data.results))
}, [])
This will prevent infinite request calls.
getList(0, 9).then(values => setPosts(values.data.results)) | ||
}) | ||
|
||
console.log('hmm') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not forget to remove logs, so they won't be in production
The deployed Netlify app has a bug #1