Skip to content
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

Danny Solis #2

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

dsolis421
Copy link

No description provided.

Copy link
Collaborator

@canaandavis canaandavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is looking pretty good.

There are some components missing propTypes, but thats not the end of the world.

Also, I like that you are persisting favorites. Though, the design here could maybe be done in a more efficient way, we'll talk more about those concepts when we look at our backend apis.

Overall great work!!

)
}

export default Activity;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see the use of propTypes here

src/App.js Outdated
axios.get('http://localhost:4000/contacts')
.then(resp => {
this.setState({
searchText: this.state.searchText,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting searchText here isn't necessary.

src/App.js Outdated
.catch(err => {
console.log(`Error! ${err}`)
})
axios.get('http://localhost:4000/favorites')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you added the ability to have favorites persisted

src/Contact.js Outdated
}
}

export default Contact;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see propTypes here also.

);
}

export default ContactList;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see propTypes here also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants