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

Naheed Edges #35

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

Naheed Edges #35

wants to merge 9 commits into from

Conversation

arangn
Copy link

@arangn arangn commented Dec 17, 2018

Inspiration Board

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Explain the steps in creating a new Card from the form. Create the form, make sure the value and name are correct, write a function to dynamically create the options for the select emoji dropdown dropdown and use that function in your form, create an onInputChange function to grab the user input, pass that to the POST request to create a new card, then send another GET request to list the new card.
How did you learn how to use the API? Reading documentation and experimenting in postman
What function did you use to place the GET request from the API to get the list of cards? Why use that function? I used .map to place the GET request in order to pass all the information into the Card component.
Explain the purpose of a Snapshot test. Comparing your changes to the existing render in order to see if your changes broke anything.
What purpose does Enzyme serve in testing a React app? Enzyme lets us manipulate and make assertions on components, and also render and manipulate the DOM.

@tildeee
Copy link

tildeee commented Dec 20, 2018

Inspiration Board

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
General
Card Component renders the data provided as props x
Board Component takes a URL and renders the list of Cards and passes in callback functions x
NewCardform Component is a controlled form and uses a callback function to return entered data to the parent component x
API
GET request made in componentDidMount x
DELETE request made in callback function x
POST request made in callback function passed to NewCardForm component. x
Snapshot testing they exist and work, but all fail!
Styling x
Overall

Good work Naheed!

It seems like all of your tests work, but need to be updated!

Other than that, I have a few comments, but otherwise your project looks good. Good work!

}

getCards = () => {
axios.get('https://inspiration-board.herokuapp.com/boards/Naheed/cards')
Copy link

Choose a reason for hiding this comment

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

it would be nice if you used the props passed into Board from App -- url and boardName!

addCard = (text, emoji) => {
axios.post(`https://inspiration-board.herokuapp.com/boards/Naheed/cards?text=${text}&emoji=${emoji}`)
.then((response) => {
this.getCards()
Copy link

Choose a reason for hiding this comment

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

instead of making another get request via this.getCards, we would prefer that you modify state

deleteCard = (id) => {
axios.delete(`https://inspiration-board.herokuapp.com/cards/${id}`, id)
.then((response) => {
this.getCards()
Copy link

Choose a reason for hiding this comment

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

like above ... instead of making another get request via this.getCards, we would prefer that you modify state

Copy link
Author

Choose a reason for hiding this comment

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

I initially tried to update state, but it wouldn't re-render with the changes. When I did a hard refresh the new card would show/delete, but it wouldn't show up on its own after using setState. I wasn't able to find a solution so I went with another get request, but any ideas why setState wouldn't render?

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

Successfully merging this pull request may close these issues.

2 participants