Design comparison
Solution retrospective
This is my first front-end mentor challenge, I look forward to reading your reviews! Thank you!
Community feedback
- @DrMESAZIMPosted over 2 years ago
Hi @SebastienWeb
This was a good attempt since its was your first challenge. My advice is to continue on that path by starting with newbie challenges.
To make your solution look better here is my suggestion
- lets reduce the width of the card class on file css/main.css line 21 to 40%
2 remove the overflow property for card class on file css/main.css line 25
Marked as helpful2@SebastienpandaPosted over 2 years ago@DrMESAZIM Thanks for your feedback! I put the overflow hidden property on the card so that it cuts the image so I don't need to put the border-radius on the image and the card and I will put 40% instead of 25%?
1 - @vanzasetiaPosted over 2 years ago
Hello, Sebastien! 👋
It's great that you write the styling with the mobile-first approach however, adding a desktop styling which in this case, only changes the background image of the
body
element on 1440px is too late.Also, on my desktop view (around 1280px width) the card is trying to fill the entire width of my browser. So, I recommend adding some
max-width
to prevent this from happening.To wrap everything here are the points that I'm trying to suggest
- First, the media query is only used for changing the background image.
- Second, to make the card responsive you only need to set a
max-width
and that's it.
I notice a
div
withrole="banner"
in the HTML which is actually the same as aheader
tag. I recommend making it as thebody
background image instead.header
is used to wrapnav
and logo element (not for the background image). I highly suggest reading the MDN documentation aboutrole="banner"
.There's no need to use
role
attribute. Why would you use it?1@SebastienpandaPosted over 2 years ago@vanzasetia Hello, I'm going to correct this and for the attributes because in the html errors that the site sent me, it told me that the roles were missing or I did not understand everything
1@vanzasetiaPosted over 2 years ago@SebastienWeb What is the site that told you about
role
attributes? The Frontend Mentor report doesn't say anything aboutrole
attributes.I highly recommend removing the
div
for the background image. You can use CSS background properties on thebody
element.Also, remove the
role
attribute from themain
element. The purpose ofrole
attribute is to make the non-semantic HTML likediv
andspan
to work as semantic HTML. However, it's best to always use to native HTML element whenever possible.0@vanzasetiaPosted over 2 years ago@SebastienWeb Where did you get the error notification?
0
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