-
Notifications
You must be signed in to change notification settings - Fork 21
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
Review latest version ! #2
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very clean and good looking code. Good job team!
just a note: I noticed the genre feature is not working, so for the future try not to include features that you are still working on it for reviewing.
Evan-Sara-Jalal/movie-app/src/App.js
Outdated
function handleSubmit(e) { | ||
//e.preventDefault(); | ||
// console.log(e.target[0].value); | ||
// console.log("it is working "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually, comments should be removed before pushing the code for the review
@@ -0,0 +1,10 @@ | |||
import React, { Component } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we need to import Component here?
overview={movie.overview} | ||
popularity={movie.popularity} | ||
release_date={movie.release_date} | ||
key={index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a short cut, you can pass the movie object to Movie component and in that component put what you want to you form it
ex:
{props.movies.map((movie) => (<Movie movie={movie} />))}
in Movie component :
<h1>{movie.original_title}</h1>
<p>{movie.release_date}</p>
and so on ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you did a great job team! loved your work.
left some comments to make your project even better!
import React, { Component } from "react"; | ||
import MovieGrid from "./movieGrid"; | ||
import { useState } from "react"; | ||
import MoviePage from "./moviePage"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete the not used imported files or hooks or anything. fix here and all.
<h2>Actors</h2> | ||
<hr /> | ||
{movie.id && | ||
cast.map((actor) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will prefer it if you used slice() with map() here, and show more info about the actor.
const history = useHistory(); | ||
const location = useLocation(); | ||
const [movie, setMovie] = useState({}); | ||
const [genres, setGenres] = useState([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the unused lines of code
movie.genre_ids.includes(parseInt(genreId)) | ||
); | ||
setMovieList(filtered); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you hit inter with an empty input will broke the code.
use
.catch((err) => {
console.log(err);
});
return ( | ||
<> | ||
<Navbar bg="dark" variant="dark" expand="sm"> | ||
<Navbar.Brand href="#home" className="text-danger"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link should take us to the main page
use to
with Link, not href
This is the first version of our movie app !!
github pages : https://evanhameed99.github.io/my-movie-app/