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

Ajmal and Giorgio project movie #322

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

Conversation

GiorgioCugusi
Copy link

No description provided.

AjmalZ and others added 4 commits March 28, 2023 17:41
created the hover for the movie title and release date in the list.css and styled the button in the details
color: black;
}

@media screen and (max-width: 800px) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code makes use of media queries to adjust the layout and styling of the page based on the screen size. This is a good approach to ensure that the page is usable and readable on different devices.

}
.details-container img {
border: solid 3px white;
scale: 95%;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSS properties used in the code seem appropriate and well-suited for their respective elements. There are some properties used that are less commonly used, such as "scale" on the image, but overall they seem to serve the purpose intended.

@@ -0,0 +1,111 @@
.movie-overview {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code organization: The code is well-organized and easy to read, with clear separation between different sections and components of the page. This makes it easy to maintain and update the code in the future

Copy link

@malvinamaria malvinamaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this code looks well-written and well-organized. It follows good coding practices and uses appropriate CSS properties to achieve the desired visual effects.

@@ -0,0 +1,49 @@
/* eslint-disable react/jsx-indent-props */
/* eslint-disable indent */
import React, { useState, useEffect } from 'react';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code looks clean and well-organized. The use of React hooks and functional components is great to see, and it's good to see the code is modularized with separate CSS files.

// `id`, meaning that the function will only be triggered when the `id` value
// changes. //
useEffect(() => {
fetch(`https://api.themoviedb.org/3/movie/${id}?api_key=986aaac98bfe3b83f202c023b975310e&language=en-US`)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Details component is using an API key directly in the fetch URL. This is generally not a good practice as it exposes the API key publicly. It's better to keep the key in a .env file and access it through an environment variable.

@@ -1,9 +1,16 @@
import React from 'react';
import { BrowserRouter as Router, Routes, Route } from 'react-router-dom'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import statements seem to be correct and well-organized.

<Router>
<Routes>
<Route path="/" element={<MovieList />} />
<Route path="/movies/:id" element={<Details />} />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Routes component, you should add a fallback Route that renders a "Page Not Found" component in case the requested URL does not match any of the defined Routes.

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.

3 participants