-
Notifications
You must be signed in to change notification settings - Fork 0
store was almost finished #2
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
base: master
Are you sure you want to change the base?
Conversation
classworks/lesson-2/store/src/App.js
Outdated
activePage: 'homePage', | ||
inputValue: '', | ||
newItems: [], | ||
willBePurchased: [], |
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.
so optimistic naming :)
You know almost 70% declines happens at shopping-cart
classworks/lesson-2/store/src/App.js
Outdated
isButton: false | ||
})*/ | ||
let item = event.target.previousElementSibling.textContent; | ||
this.state.willBePurchased.push(item); |
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.
please not mutate the state
classworks/lesson-2/store/src/App.js
Outdated
if (!this.state.inputValue) { | ||
return; | ||
}; | ||
this.state.newItems.push(this.state.inputValue); |
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.
please do not mutate the state :(
classworks/lesson-2/store/src/App.js
Outdated
return; | ||
}; | ||
this.state.newItems.push(this.state.inputValue); | ||
this.setState({inputValue: ''}); |
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.
it's much better to write it that way:
const newItems = [...this.state.newItems, this.state.inputValue]
this.setState({
newItems,
inputValue: ''
})
classworks/lesson-2/store/src/App.js
Outdated
} | ||
/* | ||
buyItemFromCart = (event) => { | ||
let item = event.target.previousElementSibling.textContent; |
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.
Hope to see PR without commented code. If someone would try to review the code, commented parts would confuse
|
||
export class OneItemBlock extends Component { | ||
render() { | ||
|
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.
please remove blank lines
const {value} = this.props; | ||
|
||
return ( | ||
<li className="list-group-item"> |
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.
please fix formatting
render() { | ||
const {buyItem} = this.props; | ||
return ( | ||
<a href="#" className="btn btn-primary" onClick = {buyItem}>Buy</a> |
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.
please fix formatting
import React, {Component} from 'react'; | ||
|
||
export class PlainButton extends Component { | ||
render() { |
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.
I think you could create 1 button for handle "success" and disable state
return ( | ||
<React.Fragment> | ||
<button className="btn btn-light" disabled>Buy</button> | ||
<h3 className="text-success">✓✓✓✓✓</h3> |
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.
Pretty sure such label should be passed via this.props.children
.
At the moment you button is not only a button. It's Button + Icon
No description provided.