@PhoenixDev22
Posted
Great work Emmilie Estabillo!
@emestabillo
Posted
@PhoenixDev22 Thanks a lot! :-)
The side-by-side layout for medium devices looked awkward for me so I opted to stick with the stacked design. What's everyone's take on the tablet layout for this project?
For devices with <667px height --> I couldn't get it to work unless I use absolute positioning. Does it make sense to still cover these devices? (Please be nice) Thanks for the feedback!
@PhoenixDev22
Posted
Great work Emmilie Estabillo!
@emestabillo
Posted
@PhoenixDev22 Thanks a lot! :-)
@marcus-hugo
Posted
Hey Emmilie, I've been googling javascript for a basic carousel, and all the tutorials are very complicated, but, your solution is so concise! I couldn't understand how to implement the logic and I've been studying your solution and wondering how the slideIndex gets updated with each click?
I think I understand. Is it because slideIndex is a variable that gets updated with each onclick="plusSlides(1)
or onclick="plusSlides(-1)
?
@emestabillo
Posted
@marcus-hugo Hi Marcus, yes, I remember using this link as a reference https://www.w3schools.com/w3css/w3css_slideshow.asp. There's a brief explanation at the end that might help :-)
@marcus-hugo
Posted
@emestabillo Thanks a lot!
@alex-kim-dev
Posted
I do basically the same as you did - leave the 1 column layout and give it max-width
to prevent content (especially text) getting too wide + centering
@emestabillo
Posted
@Alex-K1m This is another project I want to go back and edit to incorporate all the comments here. Layout-wise, I think I will still stick to one column for tablet. Side by side at medium widths seems tight to me.
@shashilo
Posted
Great job! At first glance it looks pretty darn close to the design, but I did see a few things that could use some extra attention.
I'm curious why you set the html
font-size to 38px and 32px for mobile. I usually set up my html
at a default of 10px and adjust it from there.
The content font colors look like they're not using the dark-blue
CSS variable.
You're missing the border-radius
on the images.
The body background leaks through the container. I believe it would be better if this background was on .slider
.
Other than that, I'd like to se a smoother transition instead of a display: hide/block;
.
@emestabillo
Posted
Hi @shashilo,
@rfilenko
Posted
Hi EMMILIE, the issue is with .wrapper - height:100% on mobile casues problems, just disable it, otherwise looks great on mobile🙂
Roman
@emestabillo
Posted
@rfilenko Awesome! Thank you for looking into it :-)
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord