-
Notifications
You must be signed in to change notification settings - Fork 17
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
Play Your Cards Right #6
base: master
Are you sure you want to change the base?
Conversation
super(); | ||
this.state = { | ||
url: 'https://deckofcardsapi.com/api/deck/new/draw/?count=52', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since url does not change, it can just live in a const outside of class
fetch(url) | ||
.then(response => response.json()) | ||
.then(data => { | ||
this.setState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is too much going on here. Especially with mulitple levels of callbacks. This could be done using a single setState call.
const cleanCards = this.cleanData(data.cards)
this.setState({
allCards: this.state.allCards.concat(cleanCards),
cardsReady: true
});
}); | ||
|
||
test('cardShuffle should return a randomised array', () => { | ||
const mockRandom = jest.fn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
Great work. Love the tests |
Managing state in the top level of the app and extensive conditional rendering of dumb components made this quite complex.
Some of the components are unnecessarily granular, but this was to try out props.children and making elements suchs as buttons as reusable as possible.