@Islandstone89
Posted
Hi, here is my feedback, hope it helps!
HTML:
-
You need a
<main>, this is important for accessibility. Make.containera<main>instead of a<div>. Also, you don't need to wrap anything in divs in this simple component. -
The alt text also needs to say where it leads (frontendmentor.io).
-
Headings should always be in order, so you never start with a
<h3>. Change it into a<h1>. -
.attributionshould be a<footer>. -
Footer text needs to be wrapped in a
<p>.
CSS:
-
Performance-wise, it's better to link fonts in the
<head>of the HTML then using@import. -
It's good practice to include a CSS Reset at the top.
-
Font-size must never be in px. Use rem instead.
-
If you follow my advice, and make
.containerthe card, you need to move your.containerproperties to thebody, and the.container-1properties to.container. -
bodydoes not need amin-width: 100vw- it is a block element, and is 100% wide by default. Also, never set fixed heights. Change it tomin-height: 100vh. -
The card should not have any percentages.
-
Remove all fixed widths and heights.
-
Remove
margin-topon the image. Addpaddingon all 4 sides of the card to create the necessary space. -
Add
display: blockandmax-width: 100%on image. -
Put
font-familyandtext-alignon the body, and remove it elsewhere. -
Remove the media query, there is no need for it in this challenge. When you do need to add media queries, make sure they are in rem.
Marked as helpful
@AndresDev28
Posted
@Islandstone89 Thank you very much for your advice. I've learned a lot by applying them and watching how you tackled the challenge. It's much simpler and more effective than how I did it. Additionally, I was unaware of certain aspects, such as CSS resetting.
Thanks again!
