Sammy & Camilla - Project Movies #324
Conversation
Cleaned up some css-doubles in media queries
There was a problem hiding this comment.
Overall comments:
Styling of single movie page for desktop is not super intuitive. If you check the chrome inspector and try to modify the styling, it's not super clear what the result will look like. The end product works and looks really good, so nothing major to worry about! But if you want to improve I would look in to how to make the code more intuitive here.
Generally good responsiveness. It looks good in all screen sizes with a few minor points that we have mentioned.
Good job on breaking down the components even more than the example code! It's really good practice. Really good work keeping track of all the props and styling components! :)
It looks really good! Clean design. We love the back button and the favicon!
Good job!! :)
/Matilda & Johanna
| if (!movie) { | ||
| navigate('/404'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think this is the origin of your 404 error. It seems like it does not recognize that what the user is typing in incorrectly should generate a 404. Instead of navigating to 404, it tries to insert the faulty input into the component.
Maybe this could work?:
if (!movie) { return <Navigate to="/404" replace />; }
Or see what 'movie' returns when the id does not exist.
| } | ||
|
|
||
| .movieDetHeading h2 { | ||
| height: 30px; |
There was a problem hiding this comment.
Can you put a height on a h2? With no font size? Not wrong, just wondering :)
| <meta property="og:title" content="Project Movies"> | ||
| <meta property="og:description" content="Find inspiration on what to watch next"> | ||
| <meta property="og:image" content=""> | ||
| <meta property="og:image:alt" content="Project Movies overview"> |
There was a problem hiding this comment.
Nice job adding og-tags! I think you are missing the image though?
| @@ -13,7 +20,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> | |||
| <title>Project Movies</title> | |||
| <link rel="icon" type="image/png" href="%PUBLIC_URL%/assets/icons8-clapperboard-50.png" > | |||
| @@ -1,9 +1,19 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Good job leaving the App.js component clean like this.
| <div className="details"> | ||
| <h1>{movie.title}</h1> | ||
| </div> |
There was a problem hiding this comment.
Is it a choice to not include the release date? It looks a bit like the details div is surplus. You could just put the h1 element as a sibling to img, since it is the only element inside the div "details". It's not wrong, but maybe you could simplify the code here.
| if (loading) { | ||
| return ( | ||
| <p>Loading movies...</p> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Like you mentioned during the demo, you could add the setTimeOut function to check what the loading part looks like. But not important :)
|
|
||
| useEffect(() => { | ||
| setLoading(true); | ||
| fetch('https://api.themoviedb.org/3/movie/popular?api_key=bdc909b4c34b17568b8111077d9d4d62&language=en-US&page=1') |
There was a problem hiding this comment.
We also have the API key in here hehe, but I think it should be hidden. Until we get it to work you could break out the API key as a variable, to make it easier to hide later.
| } | ||
|
|
||
| .moviePoster img { | ||
| width: 250px; |
There was a problem hiding this comment.
Not important, but the image size could be dynamic (by using % and margin/padding to control the size) for a nicer user experience on the Single Movie page. Same with the plot text in tablet and desktop.
| @@ -0,0 +1,88 @@ | |||
| .movie-details-box { | |||
| background-color: rgba(0, 0, 0, 0.6); | |||
There was a problem hiding this comment.
I like the transparent dark background in the text box, it makes it more readable! Really nice idea!
No description provided.