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

Week 9 – Movie Project – Sofia & Nathalie #48

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

Conversation

ohitsnathalie
Copy link

Netlify link

https://nathalies-sofias-project-movies.netlify.app/

Collaborators

[sofia32057]

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.

Good job with this project Sofia & Nathalie! It looks good, it works well - so be proud ⭐

export const App = () => {
return <div>Find me in src/app.jsx!</div>;
};
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

It's wonky indentation again 😞 Please ask a question for Q&A or on Stack Overflow if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it makes your code very hard to read 😅

return <div>Find me in src/app.jsx!</div>;
};
return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need the fragment? 👀

color: white;
display: flex;
flex-wrap: wrap;
/* align-items: flex-end;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused code

Comment on lines +14 to +32
useEffect(() => {
const fetchDetails = async () => {
try {
const response = await fetch(
`https://api.themoviedb.org/3/movie/${movie.slug}?api_key=${API_KEY}&language=${API_LANG}`
)
if (!response.ok) {
throw Error("Something wrong with the fetch")
}
const data = await response.json()
setResults(data)
} catch (error) {
console.error("Error", error)
setResults([])
}
}

fetchDetails()
}, [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also be written as:

const fetchDetails = async () => {
    try {
      const response = await fetch(
        `https://api.themoviedb.org/3/movie/${movie.slug}?api_key=${API_KEY}&language=${API_LANG}`
      )
      if (!response.ok) {
        throw Error("Something wrong with the fetch")
      }
      const data = await response.json()
      setResults(data)
    } catch (error) {
      console.error("Error", error)
      setResults([])
    }
  }
  
useEffect(() => {
  fetchDetails()
}, [])

Comment on lines +67 to +69
<GenreList
genreArray={results.genres}
n={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, nice! ⭐

@@ -0,0 +1,29 @@
import "./Footer.css"

export const Footer = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice idea with a footer! What do you think about scaling it down a bit, maybe make your names links to make the text a bit shorter? And maybe aligning it with the rest of the content? Aaand potentially making it stand out more from the rest of the page, e.g. changing the background color to show that it doesn't have to do with the movies per se

Skärmavbild 2024-04-10 kl  14 16 59

Choose a reason for hiding this comment

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

I second that :)

)

return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

This fragment is not needed, since the article is wrapping it all

Comment on lines +4 to +16
export const Header = () => {
return (
<header>
<nav>
<NavLink
to="/"
className="home">
HOME ›
</NavLink>
</nav>
</header>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this component needed when you have the back button on all movie pages? 👀

)}
</section>
<div className='load-more-wrapper'>
<button id='load-more-btn' onClick={loadMore}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this!

Copy link

@kathinka kathinka left a comment

Choose a reason for hiding this comment

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

Really good job! you guys finished the project with all of the requirements and then some! 🥇 Not easy to find something to improve here but I have some suggestions, who you could consider.

it was really nice to see all the different filtering, it really made the experience looking for a movie to see so much better. maybe include some more space for the different categories on mobile though as they ca be hard to click if there is many and the resolution is high.

now Im off to popping some popcorn, and watching a movie - btw: nice touch on the 404 page 💯

src/data/api.jsx Outdated
method: "GET",
headers: {
accept: "application/json",
Authorization: "Bearer 581a97ea9ebd9cf581e85b49251999f8",

Choose a reason for hiding this comment

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

Maybe delete the history in github so you key is not out in the open

Choose a reason for hiding this comment

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

It's not the active key :)

<NavLink
to="/"
className="home">
HOME ›

Choose a reason for hiding this comment

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

Maybe remove one of the home buttons?

<div className="back-btn-wrapper">
<Link to={"/"}>
<img
className="back-button"

Choose a reason for hiding this comment

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

stick to either this one or the other one. this seemes a bit more styled so maybe just keep this one?

const POSTER_URL = "https://image.tmdb.org/t/p"
const movie = useParams()
const [results, setResults] = useState()
const API_KEY = import.meta.env.VITE_DBAPI_KEY

Choose a reason for hiding this comment

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

nice hiding that key on the client side :)

filter: drop-shadow(0 0 0.4rem black);
width: 35px;
height: 35px;
filter: drop-shadow(0 0 0.4rem black);

Choose a reason for hiding this comment

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

this shadow is a bit wonky -(square shadow on a round object) maybe use a svg instead to avoid this? :)

font-size: 0.875rem;
border-radius: 3px;
font-style: italic;
background-color: rgba(0, 0, 0, 0.4);

Choose a reason for hiding this comment

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

nice with the dark background so its easier to read :)

display: flex;
flex-direction: column;
gap: 1rem;
padding: 1rem 1rem 0 1rem;

Choose a reason for hiding this comment

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

maybe add some more contrast here as well to improve readability?

@kathinka
Copy link

kathinka commented Apr 10, 2024 via email

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