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

Demo Version - includes sort & pagination #7

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

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.

None yet

2 participants