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

News update feature #11

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

Conversation

DanGRT
Copy link

@DanGRT DanGRT commented Oct 1, 2018

No description provided.

favouriteResults: []
})
//make sure we received an array in local storage before using array method to fill favouriteResults
faveList
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are not using the last part of the ternary, an if statement might be better here




fetchSearchResults(searchString, page = 1){
Copy link
Contributor

Choose a reason for hiding this comment

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

like default params


<h4>Recent News</h4>
{this.state.filmNews.map(story => {
return <NewsItem key={v4()} story={story} />
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with using uuid here is that every time it is called, it will generate a new id number, which means that React will not be able to use the id to optimise render. Another way of doing it would be to generate a uuid for each news item when it comes back from API and then assign it to each news object. By re-using precomputed ids we can then allow React to map those ids to each component it renders


handleSubmit(event){
event.preventDefault()
this.props.retrieveSearchString(this.state.textInput)
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 there is some state duplication going on here since both HeaderSearch and App store the search string in their state. It would be better to store it just in state of App and pass it down as props to HeaderSearch

@dmitrigrabov
Copy link
Contributor

Good work!

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