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

Chantelle | Edges | Inspiration Board #36

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

Conversation

BASIC-Belic
Copy link

@BASIC-Belic BASIC-Belic 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. I put state of the card attributes in newcard form, had a listener that would listen for a change in each of these attributes/fields. Once submitted, this would pass the current state into Board, using callback function passed in as a prop, and Board is responsible for speaking to card and passing on props from this obj it got from from.
How did you learn how to use the API? A combo of postman, console.log to see data/response, and reading the docs.
What function did you use to place the GET request from the API to get the list of cards? Why use that function? We placed it in componentDidMount because Axios is asynchronous, and we want the data to render on after the component has successfully mounted. It also only gets called once (not on a re-render), so for our purposes, where we don't want the data to be constantly up to date (our App is the source of truth, not the API), we don't want this to change when different components are re-rendered.
Snapshot tests? What are they good for? Absolutely... They lock in the behavior and whatever you want your components to look like (after you're done changing them), so when someone else (or you) go back and change it, they can identify whether they want to keep or disregard those changes, whether the changes were intentional, or insights into how it might affect UI-- what changes might need to be made to prevent this.
What purpose does Enzyme serve in testing a React app? It allows us to mount and traverse the React virtual DOM tree.

@droberts-sea
Copy link

Inspiration Board

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Card Component renders the data provided as props yes
Board Component takes a URL and renders the list of Cards and passes in callback functions yes
NewCardform Component is a controlled form and uses a callback function to return entered data to the parent component yes
API
GET request made in componentDidMount yes
DELETE request made in callback function yes
POST request made in callback function passed to NewCardForm component. yes
Snapshot testing yes
Styling yes
Overall Good work overall. I've left a couple of inline comments below, but in general it's clear to me that the learning goals around integrating a React app with an API have been met. Keep up the hard work!


this.state = {
cards: [],
url: this.props.url,
boardName: this.props.boardName,

Choose a reason for hiding this comment

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

You should only include things in state if they're going to change as your application runs. As far as I can tell url and boardName don't change, which means it would be clearer to leave them in props

let URL = `${this.state.url}boards/${this.state.boardName}cards`;

axios.post(URL, cardObj)
.then((response) => {

Choose a reason for hiding this comment

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

I like that you've kept all the API interaction logic in one place - the callbacks are a little more complex, but I would say it makes the app as a whole much easier to comprehend. Whether or not you intended it, this is a great example of the container component pattern well-applied.

const Card = (props) => {

const { text, emoji, id, deleteCardCallback} = props;

Choose a reason for hiding this comment

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

I like that you converted this to a functional component!

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