-
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
Project movies! #319
base: master
Are you sure you want to change the base?
Project movies! #319
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.
Good job, the page looks great. You definitely gave it your personal touch. You modified the design slightly, so extra points for your creativity! Also, all works cool with refreshing the page (when on the details page) and with the not found page.
This review was done by @smirrebinx Michelle Wegler and Maja Zimnoch
|
||
Start by briefly describing the assignment in a sentence or two. Keep it short and to the point. | ||
✓ Pair-programming |
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.
It would be nice to read more about the topic for the project (movies) + your information about pair programming :)
|
||
Describe how you approached to problem, and what tools and techniques you used to solve it. How did you plan? What technologies did you use? If you had more time, what would be next? | ||
✓ How to pass information such as product ids, or blog post titles in the URL and pick this up in React router to load dynamic content. |
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'd suggest writing more about the difficulties of the project, even as keywords, so you can remember it later on when you are going through your project (for example during the recruitment processes).
@@ -13,7 +12,8 @@ | |||
work correctly both with client-side routing and a non-root public URL. | |||
Learn how to configure a non-root public URL by running `npm run build`. | |||
--> | |||
<title>Technigo React App</title> | |||
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico" /> |
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 usage of favicon
<Route path="/" element={<List />} /> | ||
<Route path="/details/:movieId" element={<Details />} /> | ||
<Route path="/404" element={<NotFound />} /> | ||
<Route path="*" element={<Navigate to="/404" />} /> |
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 that you added the not found page :) And it works great.
navigate(-1); | ||
}; | ||
|
||
const API_URL = `https://api.themoviedb.org/3/movie/${params.movieId}?api_key=dfb43b39df4efba7f1f4678ddc567fa1&language=en-US&page=1`; |
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.
It's nice that you managed to make it work with the variable API_URL
<div className="movie-item-info"> | ||
<h2>{movie.title}</h2> | ||
<p>⭐️ {Math.round(movie.vote_average * 10) / 10} on IMDB</p> | ||
<span className="released">Release date: {movie.release_date}</span> |
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.
To have the date written in a nicer format, you can check a 'moment' node module.
.finally(() => setIsLoading(false)); | ||
}, [API_URL]); | ||
|
||
if (isLoading) { |
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 wonder why you used the Loading component only for the Details page.
<div className="movie-list"> | ||
{movieList.map((movie) => { | ||
return ( | ||
<NavLink className="movie-item" key={movie.id} to={`/details/${movie.id}`}> |
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.
What made you use NavLink instead of Links?
@@ -1,13 +1,155 @@ | |||
@import url('https://fonts.googleapis.com/css2?family=Montserrat:wght@300;400;500;600;700&display=swap'); |
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.
Bravo for importing a new font!
.truncated { | ||
overflow: hidden; | ||
white-space: nowrap; | ||
text-overflow: ellipsis; |
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.
Thank you for using this, interesting property! Is it for the text of the description of the movie?
https://glistening-tiramisu-f31c45.netlify.app/