Design comparison
Solution retrospective
Any feedback is more than welcome, also if you have any questions feel free to ask, peace <3
Community feedback
- @pikapikamartPosted about 3 years ago
Hey, awesome work on this one. Desktop layout looks great, it is responsive and the mobile state looks great as well.
Some suggestions would be:
- 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. - Don't
position: absolute
thefooter
since it just goes in the way when opening up dev tools at the bottom. Remove it then applyflex-direction: column
in thebody
tag. - Those car icons are only decoration. 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 the image is usingsvg
. - Only use descriptive
alt
on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users. - When using
img
tag, you don't need to add words that relates to "graphic" such as "icon" and others, sinceimg
is already an image so no need to describe it as one. - Don't need to add
header
andfooter
on eacharticle
though but I see that you're just using best practice inarticle
:> But remember that landmarks such asarticle
section
needs to have a heading tag inside it as its text. On this, theheader
wraps the heading which is of course not for thearticle
. So maybe usingdiv
for this one would be better^^ - Also, I wouldn't use the
h1
as the title car on this one and instead use a screen-reader onlyh1
. Have a look at Grace's solution on this challenge. Inspect the usage of theh1
on this as well the stylings that are applied on it. - When wrapping up a text-content, make sure that it is inside a meaningful element like
p
tag or heading tag and not using likediv
to wrap the text.
Aside from those, great job again on this one.
Marked as helpful0 - Avoid using
- @ohsitePosted about 3 years ago
Hi @pikamart, Thank you for your support! As for the body min-height that's exactly what I've been missing! Giving it just height on mobile the content would overlap top and bottom but stay centered. Really big thx for this solution. As for the footer I position it relative to take it out of the DOM order. Making it child of flex container therefore occupying some space in it, will make the second element not centered. Unless....I maybe give a footer margin top auto...yea that might work! As for the screen readers, semantic tags, and arias that is a definitely my weak side, that I have to work on more. If you be so kind please follow my future challanges and share your feedback! I've learned a lot from you. More than form few hours course! Thx again, peace <3
1@pikapikamartPosted about 3 years ago@ohsite Hey, really great that you find it useful^^
Sure, i'll give you a follow so that your projects could appear in my feed! Looking forwards to those challenges^^
Marked as helpful1
Please log in to post a comment
Log in with GitHubJoin 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