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

Movies Sofie #67

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

Movies Sofie #67

wants to merge 27 commits into from

Conversation

SofieFerrari
Copy link

@SofieFerrari SofieFerrari commented Apr 9, 2024

@HIPPIEKICK
Copy link
Contributor

Can you share a link to your deployed site? 😇

@SofieFerrari
Copy link
Author

Copy link

@Caracal23 Caracal23 left a comment

Choose a reason for hiding this comment

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

Way to go Sofie!! 🚀
It is great to see how you manage this solo project; you should be proud of yourself!
Keep it up with the good code !!
-Izabel&Etna

<div
className="detail-background"
style={{
backgroundImage: `url(https://media.themoviedb.org/t/p/w780${detailData.backdrop_path})`,

Choose a reason for hiding this comment

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

Since there are screens that are bigger than 780; maybe next time you could consider on taking 1280px for background images.
-Izabel&Etna

//nedan är försök att få animationen att visas längre.

// useEffect(() => {
// export const DelayLottie = setTimeout(() => {

Choose a reason for hiding this comment

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

For the next time you might want to consider creating the function "DelayLottie" before (for example, around line 5) and then you can call the function inside of the useEffect. 👍
-Izabel&Etna

/>
</div>
</div>
</Link>

Choose a reason for hiding this comment

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

Code looks great but it gets a bit complicated to read due to the unorganised set up.
-Izabel&Etna

Choose a reason for hiding this comment

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

It is interesting how you split in two different components the fetching and the data rendering. Maybe you can explain us why you choose that logic 😊.
-Izabel&Etna

.movie-wrapper {
display: flex;
flex-wrap: wrap;
justify-content: flex-end;

Choose a reason for hiding this comment

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

Just a question here...Did you want to present the movies all the way to right? or you want them to be center?
-Izabel&Etna

yaout and made it easier to read text by creating hover effect
Copy link
Contributor

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Please check:

@SofieFerrari
Copy link
Author

Please check:

Hi Antonella!
Thanks for checking!
I get 100% in both list and detalis now :D

Copy link
Contributor

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Hi Sofie, getting there, please deploy the routes adding the netlify.toml file and also format the files 🪄

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