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

My React Cinema #16

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

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.

None yet

2 participants