Skip to content

Homework 2 #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

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

Homework 2 #2

wants to merge 3 commits into from

Conversation

MShcherbyna
Copy link
Collaborator

No description provided.

}

addToCart = (product) => {
let self = this;
Copy link
Member

Choose a reason for hiding this comment

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

you could use arrow function instead of save context to additional variable


this.state.inCart.push(item);

this.setState({
Copy link
Member

Choose a reason for hiding this comment

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

such update could looks that way

this.setState({
 inCart: [...this.state.incart, product]
})

let self = this;
let item = this.state.products.find(function(item, index){
if (item.name == product.name) {
self.state.products[index].inCart = true;
Copy link
Member

Choose a reason for hiding this comment

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

if you have inCart in state and inCart in the object it seems like a duplication an additional overhead
Furthermore, it breaks one of principle "Single Source of Truth"

export class Admin extends Component {
render() {

return (
Copy link
Member

Choose a reason for hiding this comment

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

please use prettier for code formatting

<h1>ADMIN PAGE</h1>
<ul className="list-group">
{
this.props.products.map(product => {
Copy link
Member

Choose a reason for hiding this comment

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

it worth to move such logic to class method

{
this.props.products.map(product => {
let inCart = product.inCart ? (
<div>
Copy link
Member

Choose a reason for hiding this comment

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

it worth to use React.Fragment there instead of extra div

<div className="row">
{
this.props.products.map(product => {
let inCart = product.inCart ? (
Copy link
Member

Choose a reason for hiding this comment

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

also, you could compare product inside inCart state property instead of check property of item

<h3 className="text-success">✓✓✓✓✓</h3>
</div>
) : (
<a href="#" className="btn btn-primary" onClick={() => this.props.addToCart(product)}>Buy</a>
Copy link
Member

Choose a reason for hiding this comment

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

please move onClick to class method

<div>
User
</div>
<button onClick={this.props.changePageToCart}>Orders</button>
Copy link
Member

Choose a reason for hiding this comment

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

please use object destruction

@@ -0,0 +1,20 @@
{
"name": "youtube",
Copy link
Member

Choose a reason for hiding this comment

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

please move all youtube specific homework to another PR

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