Skip to content

Conversation

@tigershk
Copy link

No description provided.

@tigershk
Copy link
Author

Readme.md conflict

text-align:center;
}

input {
Copy link
Contributor

Choose a reason for hiding this comment

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

be careful around applying styling directly on input tags. there are different input types and some will require different styling approaches. If you styling just text inputs, use a class name and apply it to components you wish to style

margin:0px
}

.App__header{
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using caps in CSS class names

receiver(results) {
movieArray.unshift(results);
this.setState({ movies: movieArray });
console.log(this.state.movies)
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid committing console.logs. Also, this console.log is likely to run before setState has had an effect as it is asynchronous. To console.log after setState has taken effect you can use

  this.setState({ movies: movieArray }, () => console.log(this.state.movies));

Where the second parameter passed to setState is a callback which will be called after setState has completed.

//receive user-clicked movie to set more info state variables
receiveMovie(movieID, plot) {
this.setState({
["currentMovie"]: movieID,
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 use currentMovie as key as it's not a variable

sortBy: event.target.name
})

const sorted = (event.target.name === "Rating") ? this.state.movies.sort((a, b) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This call will mutate state. It would be better to create a clone of this.state.movies, sort it, then set it back in state using setState.

{this.props.currentMovie}
<br />
{this.props.currentInfo}
{/* <strong>Age Rating: </strong>{this.props.movie.Rated} <strong>Runtime: </strong>{this.props.movie.Runtime} <br />
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid committing commented out code

}

toggleInfo() {
this.setState({ infoHidden: !this.state.infoHidden })
Copy link
Contributor

Choose a reason for hiding this comment

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

nice


nextPage() {
this.setState({ searchMovie: this.state.nextMovie });
this.setState({ pageNum: this.state.pageNum + 1 }, () => this.fetchMovie());
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

//receiver function to populate movies in state
receiver(results) {
movieArray.unshift(results);
this.setState({ movies: movieArray });
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 avoid using an external array to store temporary results and do the operation using state. Let me know if you want any help getting this to 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