@pikapikamart
Posted
Hey, awesome work on this one. Layout in desktop seems fine, the responsiveness could be better since right now, the container shifts based on the text-wrapping right. The mobile layout could use some more width
.
Grishmita already said some useful feedback, just going to give some suggestions as well.
- Always have a
main
element to wrap the main content of the page. The.container
selector could have usedmain
instead ofdiv
. This way, contents are inside a landmark element. - Avoid using
height: 100vh
on a large container like your.container
selector. This limits the element's height based on the screen. Usemin-height: 100vh
instead, this takes full height as well but allows the element to expand if needed. - For this one , I would put the
.attribution
inside thefooter
tag, so your markup should look like:
<main />
<footer />
- Avoid using multiple
h1
on a page. Use only at least 1h1
, change those intoh2
. But in a page, you need anh1
for this one, you will make theh1
a screen-reader only text by having ansr-only
stylings. This means, the text will be hidden visually but visible for screen-readers. Theh1
text-content should describe what is the content of themain
is all about. Have a look at Grace's solution on this one inspect it and see how she used theh1
and copy as well the styling applied on it, you will used that a lot. - Each car icons could use
aria-hidden="true"
attribute so that the image will be visible for all tech. - Lastly, just those that I mentioned at the beginning, the responsiveness and mobile layout.
Aside from those, great work again on this one.
Marked as helpful
@dragoshcode
Posted
@pikamart thank you so much for this code review ๐ took down the notes โ