-
Notifications
You must be signed in to change notification settings - Fork 137
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
base: main
Are you sure you want to change the base?
Movies Sofie #67
Conversation
Can you share a link to your deployed site? 😇 |
There was a problem hiding this 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})`, |
There was a problem hiding this comment.
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(() => { |
There was a problem hiding this comment.
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
src/components/PopularList.jsx
Outdated
/> | ||
</div> | ||
</div> | ||
</Link> |
There was a problem hiding this comment.
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
src/components/SecondPage.jsx
Outdated
There was a problem hiding this comment.
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
src/styling/PopularList.css
Outdated
.movie-wrapper { | ||
display: flex; | ||
flex-wrap: wrap; | ||
justify-content: flex-end; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check:
- accessibility: needs to score at least 95
- how to deploy routes
Hi Antonella! |
There was a problem hiding this 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 🪄
https://bucolic-kangaroo-37b00f.netlify.app/