@pikapikamart
Posted
Hey, great work on this one. Desktop layout looks fine, it is responsive though the upper part when going into like tablet size is being hidden (suspecting vh in here), mobile state is fine though.
Rafael already gave feedback on this one, just going to add some suggestions as well:
- Don't use
width: 100vw
since this will only add a horizontal scrollbar at the bottom, since this value does not account the vertical scrollbar's width. - Avoid using
height: 100vh
on a large container like thebody
as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to. - Inside the
main
, why isolate the 3rd box and wrap the 1st and 2nd box inside of anotherdiv
? You can just made use of:
main
>div
>div
>div
section
in here is not needed and also,section
as landmark is not informative unless it is labelled byaria-labelled
by attribute. Changing it to using onlydiv
would be better.- Those car icons are only decorative. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if you are usingsvg
instead ofimg
tag. - Only use descriptive
alt
on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users. - On a site, always have a single
h1
. Since there are no visible text that are suitable to beh1
, theh1
would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, theh1
would have likesr-only
class and the text-content should describe what is the main-content is all about. Theh1
could be placed as the first text inside themain
.Have a look at Grace's solution on this one inspect the layout and see the usage ofh1
as well the stylings applied to it. - Using
a
tag would be better for the learn-more since on a real site, those would be page links where user can "learn" more about a specific car.
Aside from those, great job again on this one.
Marked as helpful
@GodILoveTequila
Posted
@pikapikamart Thanks a lot! This was super helpful, i really appreciate it! The reason why i added the top div and bottom div is so that i can make a media query for something in between phone and desktop size screens because it didn't liked the way the cards were shrinking in size.