-
Notifications
You must be signed in to change notification settings - Fork 417
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
Week 8 '23 Movie Project Vio & Emma #321
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.
Review by Frida @Cheroptera and Josefin
Really nice touch with the retro feel style, fun that you chose to do something that is your own. We are reminiscing about VHS tapes.
Your code is very well structured and easy to read, with apt names. Good choice putting the URL:s in their own component, it adds to readability. The responsiveness looks good too.
We noticed a potential glitch with the API you used, see comment on Details component.
You've done a great job, hard to find anything to give constructive criticism on. Keep it up!
<img className="poster" src={`https://image.tmdb.org/t/p/w342${movie.poster_path}`} alt={movie.title} /> | ||
<div className="movie-text"> | ||
<h2>{movie.title}</h2> | ||
<p><strong>Release date:</strong> {movie.release_date}</p> |
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.
We noticed that the List page and the details page show different release dates, even though you seem to be fetching from the same place in the API. Any idea why this is happening?
|
||
const Footer = () => { | ||
return ( | ||
<footer> |
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.
Nice feature with the logo, adds to the overall retro feel styling. in desktop version it might be a little difficult to find the back button since nothing indicates you can scroll down.
return ( | ||
<footer> | ||
<img className="logo" src="/tmdb-logo.svg" alt="TMDB logo" /> | ||
<p>This product uses the TMDB API but is not endorsed or certified by TMDB.</p> |
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.
Good that you thought about mentioning this
width: 100%; | ||
} | ||
|
||
/*styling for movie list*/ |
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.
Helpful way for others who view your code (and probably yourselves) to structure the CSS with headlines
@@ -11,3 +18,225 @@ code { | |||
font-family: source-code-pro, Menlo, Monaco, Consolas, "Courier New", | |||
monospace; | |||
} | |||
|
|||
main { |
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.
Are these styling rules used anywhere? We didn't find a main tag.
} | ||
|
||
.movie-text { | ||
display: block; |
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.
Are the flex rules necessary here?
No description provided.