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

Project Movies by Michelle Wegler & Maja Zimnoch #313

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

Conversation

majazimnoch
Copy link

Linkt to the deployed page:
https://michelle-maja-movies.netlify.app/

Copy link

@SandraMadeleine SandraMadeleine left a comment

Choose a reason for hiding this comment

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

Code review by Ylva Karlsson and Sandra Olsson.
Very nice job with this weeks project, your page looks great and the code is clean and easy to follow. We must say that we want to instantly eat popcorn when seeing your icon, nomnom!

import NotFound from 'components/NotFound';
import Details from 'components/Details';
import ListMovies from 'components/ListMovies';
import { LIST_URL } from 'utils/urls.js';

Choose a reason for hiding this comment

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

Nice way of solving the url!

/* eslint-disable max-len */
import React, { useEffect, useState } from 'react';
import { useParams } from 'react-router-dom';
import '../details.css';

Choose a reason for hiding this comment

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

Good job with making separate css-files!

<div className="title-rating-text">
<h1 className="title-rating-container">
<span className="title-details title-details-margin">{movieDetail.title}</span>
<span className="rating">⭐{Math.round(movieDetail.vote_average * 10) / 10}</span>

Choose a reason for hiding this comment

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

If you would have wanted to have the rating as out of 10, you could add the text /10 after the last curly bracket and before the closing bracket.

import '../details.css';

// Define the Details component
const Details = () => {

Choose a reason for hiding this comment

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

We noticed that when refreshing your details page, we get the Netlify error page. We have the same problem and do not know why this happens. If you find the problem, please let us know.

import React from 'react';
import { NavLink } from 'react-router-dom';
import GitIcon from '../assets/github.png';
import MovieIcon from '../assets/movie.png';

Choose a reason for hiding this comment

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

Super cute icon! We're stealing the idea!

@@ -0,0 +1,26 @@
import React from 'react';
import { NavLink } from 'react-router-dom';
import GitIcon from '../assets/github.png';

Choose a reason for hiding this comment

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

Another way of solving this if you would like more social media icons is to import Font Awesome icons (like we did but we put it in the footer). We wanted to post code but for some reason it does not display in the comment?

To make the icons show one also has to import them in the top of the component. If you would like to know more then we can discuss it on the Monday session!

<img className="cover-image" src={movie.poster_path ? `https://image.tmdb.org/t/p/w342${movie.poster_path}` : ''} alt="poster" />
<div className="hover-container">
<h1 className="text-hover">{movie.original_title}</h1>
<p className="text-hover">Released: {moment(movie.release_date).format('D MMMM YYYY')}</p>

Choose a reason for hiding this comment

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

Nice way of showing the release date!

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