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

Frida & Axel #65

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Frida & Axel #65

wants to merge 8 commits into from

Conversation

fridaforser
Copy link

@fridaforser fridaforser commented Apr 8, 2024

Netlify link
https://newmoviereleases.netlify.app/

Collaborators
[AHPIXI]

Copy link

@Tejpex Tejpex left a comment

Choose a reason for hiding this comment

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

The site looks good and works well! Extra plus for no scrolling on the movie-info-page :)

<>
<BrowserRouter>
<main>
<div>
Copy link

Choose a reason for hiding this comment

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

Why an extra div, when "main" is there to wrap the content?

<h1 className="title">{movie.title}</h1>
<h4 className="release-date">Released {movie.release_date}</h4>
</div>
<Link key={movie.id} to={`/movie/${movie.id}`}>
Copy link

Choose a reason for hiding this comment

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

Is a key needed here, when there is one in the "movie-box"-div?

import { useEffect, useState } from "react";
import "./Home.css";

export const Home = () => {
Copy link

Choose a reason for hiding this comment

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

There is an export both here and a default export in the end. Only one is needed.

Comment on lines +65 to +79
@media (min-width: 500px) AND (max-width: 699px) {
.movie-boxes {
grid-template-columns: 1fr 1fr;
}
}

@media (min-width: 700px) AND (max-width: 999px) {
.movie-boxes {
grid-template-columns: 1fr 1fr 1fr;
}
}

@media (min-width: 1000px) {
.movie-boxes {
grid-template-columns: 1fr 1fr 1fr 1fr;
Copy link

Choose a reason for hiding this comment

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

Good that there's control of how many movies are shown and that the margin/gap (which is 0) is kept consistent.

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Well structured and well-styled ⭐ Good job!

}

.movie-info-box {
position: fixed;
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably meant position: absolute; here? You can't scroll to see all the text in mobile

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