Ada pets api solution #3
Conversation
package.json
Outdated
| "scripts": { | ||
| "start": "react-scripts start", | ||
| "server": "json-server --watch src/data/pets.json", | ||
| "server": "json-server --watch src/data/pets.json --port 2999", |
There was a problem hiding this comment.
I don't love changing the port this runs on since it will run on port 3000 in Heroku. For the solution I'd rather change the port that the node server runs on. (It looks like you're making calls to localhost:2999.)
There was a problem hiding this comment.
Ok, I can change the node server to run on 2999 and let json server run on 3K
src/App.js
Outdated
| .then((response) => { | ||
| this.setState({ | ||
| petList: response.data, | ||
| originalPets: response.data, |
There was a problem hiding this comment.
I know you didn't add it but what do we use originalPets for that we couldn't just use petList for? (Since it looks like you're setting originalPets every time you set petList.)
There was a problem hiding this comment.
It's used for filtering.
There was a problem hiding this comment.
I did change the name however to fullList
There was a problem hiding this comment.
I was actually just reading about how this is an antipattern according to react documentation.
It's storing more data in state than needs to be stored, cause it could just be storing the filter term in state, right?
There was a problem hiding this comment.
That said, I know it was already like this so maybe that's out of scope for this change.
The thing I read was step 3 here: https://reactjs.org/docs/thinking-in-react.html
There was a problem hiding this comment.
^ This would involve lifting the state out of search bar.
There was a problem hiding this comment.
@jmaddox19
You're right in that we should instead be lifting state out of the search bar and then using that term to filter the prop passed into the PetList.
There was a problem hiding this comment.
I've updated it to lift the search form's state up.
|
I have that one comment but other than that, looks good to me :) |
CheezItMan
force-pushed
the
ada-pets-api-solution
branch
from
c455313 to
67cfcc6
Compare
Warning DO NOT MERGE
This is to allow instructors to review the solution for the two API exercises. Take a look and let me know any changes that need to be made.