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

ProductList code review #3

Open
osminosm opened this issue Apr 9, 2019 · 0 comments
Open

ProductList code review #3

osminosm opened this issue Apr 9, 2019 · 0 comments

Comments

@osminosm
Copy link

osminosm commented Apr 9, 2019

note : usually this isn't done as an issue, these remarks should be done before merging from your feature branch to the develop branch, but until we have that, i'll post remarks as issues.

here
you shouldnt provide the previous state when updating the state, the following would be sufficient:
this.setState({refresh:true});

here
the variable count is used to init the state a single time when its 0 then it gets incremented to 1, i guess there is no use in using this whole variable as you can initialize your state for the first time in the constructor like this :

constructor(props){
  super(props);
  this.state={
    allProducts: this.props.products,
    ... // rest of default state
  }
}

and you can remove the componentDidUpdate logic.

here
Its useless to pass the products as parameter, since the function has access to this.state.currentProducts, remove the count conditional, and perhaps it should be a separate stateless component, not a method on ProductList.

here

return a descriptive message that there aere no products, instead of null;

remove unnecessary comment here

and think about defining propTypes for all the compoents

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

No branches or pull requests

1 participant