Skip to content

Conversation

@philberryman
Copy link

No description provided.

console.log(this.state.favouritesObject)
this.state.favouritesObject.hasOwnProperty('test')?delete this.state.favouritesObject.test:null
const favouritesKeys = Object.keys(this.state.favouritesObject);
this.state.favouritesArray = favouritesKeys.map(key => this.state.favouritesObject[key])
Copy link
Contributor

Choose a reason for hiding this comment

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

don't assign to state directly, use setState

}
this.setState({
favouritesObject:favObject
},localStorage.setItem('reactFavourites', JSON.stringify(this.state.favouritesObject)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the localStorage call from callback, by passing it favObject rather than this.state.favouritesObject

React cinema app
<div className="app">
<div className="top">
<p className="top__favourites" onClick={this.showFavourites}>Favourites ({this.state.favouritesLength})</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than storing favouritesLength in state, it would be better to use Object.values(this.state.favouritesObject).length. That will ensure that you always working from the same source of truth rather than having to recalculate favouritesLength.

// Create an array of favourites from favourites object. This is used to update the App/result state which will then show favourites instead of search

updateFavouritesArray() {
console.log(this.state.favouritesObject)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like favouritesArray is only being used to favouritesLength. I'd say neither proeprty is needed in state as both can be produced from favouritesObject

constructor() {
super();

this.state= {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like query is being stored both in Search and App. Rather than duplicating state it would be better to pass the query down as props from App into Search

@dmitrigrabov
Copy link
Contributor

Good work. There is some state duplication that can be avoided. Take a look at the comments for details

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