Skip to content

Conversation

@KateMhq
Copy link

@KateMhq KateMhq commented Oct 1, 2018

No description provided.

render(){
let check={fav_on:false};

this.props.favList.find(movie => movie.imdbID==this.props.id)? check={fav_on:true}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ternary return a value. As a result it would better to rewrite above as const check=this.props.favList.find(movie => movie.imdbID==this.props.id)? {fav_on:true}:{fav_on:false}

Copy link
Contributor

Choose a reason for hiding this comment

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

Better still, since we use check in cx, we can simplify above to const favOn=this.props.favList.find(movie => movie.imdbID==this.props.id); and then use it as
const classes=cx('material-icons md-24',{fav_on: favOn}); in cx


receiveFavClick(id){

if(this.state.favList.filter(movie => movie.imdbID==id).length == 0){
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cleaner to calculate to generate a favList array in if/else, then you can set it in state and localStorage after and you would not need to duplicate code. Also, since favList would be precomputed we would not need to use a callback when setting it in localStorage


receiveDeleteClick(id){
this.setState({
favList: this.state.favList.filter(movie => movie.imdbID !== id),
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be better to create a favList array first then set it both in state and localStorage without using the callback

}

render(){
const storageString=localStorage.getItem('favList')
Copy link
Contributor

Choose a reason for hiding this comment

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

It would better to use favList from state rather than getting it from localStorage. favList in state should have same items.

@dmitrigrabov
Copy link
Contributor

Nice work. A few bits could be a slightly more concise or tidied up but very nice work overall

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