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

Lina and Emilia Movie Site #317

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

Conversation

LinaAdamsson
Copy link

No description provided.

Copy link

@fannystenberg fannystenberg left a comment

Choose a reason for hiding this comment

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

Your code is easy to follow and your site is working as it should + follows the design given.
Well done this week you guys! 🤩

Comment on lines +1 to +4
export const API_KEY = 'c9a6004aae28b42f1163022266b85fb5';
export const LIST_URL = `https://api.themoviedb.org/3/movie/popular?api_key=${API_KEY}&language=en-US&page=1`;
export const DETAILS_URL = (movieId) => `https://api.themoviedb.org/3/movie/${movieId}?api_key=${API_KEY}&language=en-US`;
export const CREDITS_URL = (movieId) => `https://api.themoviedb.org/3/movie/${movieId}/credits?api_key=${API_KEY}&language=en-US`;

Choose a reason for hiding this comment

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

Really nice way of keeping the other components doing the fetch more "clean"!

Comment on lines +7 to +9
<meta property="og:title" content="Lina and Emilia movie site" >
<meta property="og:description" content="Top 20 popular movies right now">
<meta propert="og:image" content="🎬">

Choose a reason for hiding this comment

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

Nice that you added some og tags!

Comment on lines +36 to +44
<span className="tagline">{movie.tagline}
</span>
<span className="overview">{movie.overview}
</span>
<Credits />
<span className="rating">⭐️ {Math.round(movie.vote_average * 10) / 10}
</span>
<span className="length">{movie.runtime} min
</span>

Choose a reason for hiding this comment

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

What is the reason behind using span here instead of paragraph?
(I'm thinking that it might effect screen readers in a negative way)

Comment on lines +1 to +37
import React, { useState, useEffect } from 'react';
import { Link } from 'react-router-dom';
import { LIST_URL } from '../data/Url';
import '../css/MoviesList.css'

export const MoviesList = () => {
const [movies, setMovies] = useState([])
const [loading, setLoading] = useState(false)

useEffect(() => {
setLoading(true)
fetch(LIST_URL)
.then((res) => res.json())
.then((json) => {
setMovies(json.results)
setTimeout(() => setLoading(false), 200)
})
}, [])
console.log(movies)

if (loading) {
return <p className="loader">Loading</p>
}

return (
<section className="movies-container">
{movies.map((movie) => (
<Link key={movie.id} to={`/movies/${movie.id}`}>
<div className="movie-overlay">
<img src={`https://image.tmdb.org/t/p/w342${movie.poster_path}`} alt={movie.title} className="movie-poster" />
<h1 className="movie-text">{movie.title}</h1>
</div>
</Link>
))}
</section>
)
}

Choose a reason for hiding this comment

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

Clean code with really good structure!

Copy link

@FionaKlacar FionaKlacar left a comment

Choose a reason for hiding this comment

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

Great job! Your code is really clean overall and it's cool to see a different API endpoint used and to learn about 'slice' in the credits component! Well done!

@@ -0,0 +1,6 @@
This favicon was generated using the following font:

Choose a reason for hiding this comment

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

Nice touch with the favicon!

margin-bottom: 20px;
}

.back-button:hover {

Choose a reason for hiding this comment

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

Love the hover effect on the button!

console.log(movies)

if (loading) {
return <p className="loader">Loading</p>

Choose a reason for hiding this comment

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

When the page is loading it also shows the footer high up on the page which, so maybe the footer needs to be in a fixed position at the bottom of the page?

const Footer = () => {
return (
<section className="footer-container">
<span className="text">Made by Emilia Saberski and Lina Adamsson Technigo 2023</span>

Choose a reason for hiding this comment

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

Is there a reason this a span and not a paragraph?

Comment on lines +34 to +35
width: 20vw;
height: 4vh;

Choose a reason for hiding this comment

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

Interesting to see these units being used, and to see a button be responsive in size!

@@ -1,9 +1,22 @@
/* eslint-disable react/react-in-jsx-scope */
/* eslint-disable */
import React from 'react';

Choose a reason for hiding this comment

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

Your app component is nice and clean!

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.

4 participants