PhoenixDev22• 16,990
@PhoenixDev22
Posted
Great work Emmilie Estabillo!
1
P
Emmilie Estabillo• 5,540
@emestabillo
Posted
@PhoenixDev22 Thanks a lot! :-)
0
Design comparison
Solution retrospective
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!
Community feedback
Marcus Hugo• 365
@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)?
1
P
Emmilie Estabillo• 5,540
@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 :-)
1
Marcus Hugo• 365
@marcus-hugo
Posted
@emestabillo Thanks a lot!
1
Alex Kim• 1,325
@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
1
P
Emmilie Estabillo• 5,540
@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.
1
Shashi Lo• 1,345
@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
htmlfont-size to 38px and 32px for mobile. I usually set up myhtmlat a default of 10px and adjust it from there. -
The content font colors look like they're not using the
dark-blueCSS variable. -
You're missing the
border-radiuson 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;.
1
P
Emmilie Estabillo• 5,540
@emestabillo
Posted
Hi @shashilo,
- I was going by the style guide - the base font was 32px. I probably went backwards with this since I went mobile-first. For medium devices, I was still using the same stacked layout. So to save myself another x-lines of code trying to adjust each element, I just increased the root size.
- I missed this bit
- Another great catch! Will edit.
- I was thinking the same thing, but did it on the body anyway to match the desktop view. I'm probably going back to adjust the slider. Thank you for the pointers!!
0
Roman Filenko• 3,335
@rfilenko
Posted
Hi EMMILIE, the issue is with .wrapper - height:100% on mobile casues problems, just disable it, otherwise looks great on mobile🙂
Roman
1
P
Emmilie Estabillo• 5,540
@emestabillo
Posted
@rfilenko Awesome! Thank you for looking into it :-)
0
Please log in to post a comment
Log in with GitHub
Join our Discord community
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