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

Sam Berk -- Octos #26

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

Sam Berk -- Octos #26

wants to merge 19 commits into from

Conversation

samanthaberk
Copy link

Inspiration Board

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Explain the steps in creating a new Card from the form. The NewCardForm manages its own state, which includes 'text' and 'emoji'. The form has a text input and select input, which both have listeners 'onChange' attached to them. This click listener calls the 'onInputChange' function that listens for the event and creates a new object called 'updatedInput' that holds the name and value of the targeted event (text and optional emoji), and sets this to the current state. The form also has an 'onSubmit' click listener that calls the onFormSubmit function, which calls the addCardCallback function from props to create a new instance of the card component with the current state of the NewCardForm and then resets the state to empty strings.
How did you learn how to use the API? To test get requests, I pasted the urls into the browser to see the json that was returned. To test the post and delete requests for cards I used 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 'componentDidMount()'. This is necessary because a react app renders components asynchronously. The function 'componentDidMount() is one of the methods that React provides to us that is tied to the lifecycle of specific components and allows us to control what happens when each individual part of the application renders, or before/after the component renders. After all elements of the page are rendered, react calls 'componentDidMount() to fetch the data from the external API. We also call methods such as setState() within componentDidMount() to change the state of the application and render the JSX with the updated data from the API.
Explain the purpose of a Snapshot test. To make sure the view/UI has not changed unexpectedly when you change something in the application.
What purpose does Enzyme serve in testing a React app? Enzyme allows you to test a shallow rendering of an API, which will test only a single component. This is useful if you have a large component with many child components. When you use mount for testing, if you change one small thing in a child component the entire component would fail the snapshot test.
Summary I appreciated having so much time to work on the project this week. I feel like I have a better grasp on React because I had more time to spend delving deeper into concepts like the lifecycle methods and testing. Note- I did not get around to testing the dropdown component, but will return to this time permitting.

@CheezItMan
Copy link

Inspiration Board

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions Check
General
Card Component renders the data provided as props Check
Board Component takes a URL and renders the list of Cards and passes in callback functions Check
NewCardform Component is a controlled form and uses a callback function to return entered data to the parent component Check
API
GET request made in componentDidMount Check
DELETE request made in callback function Check, but you're reloading the entire page when that completes, this defeats the purpose of an SPA
POST request made in callback function passed to NewCardForm component. Check
Snapshot testing Some event testing errors, but otherwise it works
Styling Overall not bad
Overall You have some bugs in your code and a number of inefficiencies. See my inline notes. Most concerning is that you are reloading the page which defeats the purpose of React. That said, you hit most of the learning goals.

<Card id={0} text="" emoji="" deleteCardCallback={callback}/>
);

card.find('button').simulate('click');

Choose a reason for hiding this comment

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

This test is failing because you have a confirmation dialog as well. You need to research how to mock that.

this.setState({
cards: updatedCards
});
window.location.reload();

Choose a reason for hiding this comment

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

You're causing the entire page to reload which defeats the entire purpose of doing an SPA.

render() {
const cards = this.state.cards.map((card, index) => {
return (
<div>

Choose a reason for hiding this comment

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

Since this function is returning a collection of Cards, you need to include a key attribute.

Something like:

<div key={index}>


id: PropTypes.number.isRequired,
text: PropTypes.string.isRequired,
emoji: PropTypes.string.isRequired,

Choose a reason for hiding this comment

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

It's either the emoji is required or the text, but not necessarily both!

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