@Islandstone89
Posted
Hi, here is some feedback - hope it helps :)
HTML:
-
You don't need a
<header>in this challenge, only a<main>and a<footer>. Move the image into the<main>. You don't need any divs, you can use the<main>as the card. -
The alt text also needs to say where it leads (frontendmentor.io).
CSS:
-
Performance-wise, it's better to link fonts in the
<head>of the HTML than using@import. -
It's good practice to include a CSS Reset at the top.
-
font-sizemust never be in px. This is bad for accessibility, as it prevents the font size from scaling with the user's default setting in the browser, Use rem instead. -
On the
body, changeheighttomin-height. -
Remove the fixed widths and heights. You rarely want to set fixed dimensions, as this easily creates issues with responsiveness.
-
Add a
max-widthof around 20rem on the card, to prevent it from getting too wide on larger screens. -
Since all of the text is centered, you can set
text-align: centeron thebody, and remove it elsewhere. The children of thebodyinherit the value. -
It's common to add
display: blockandmax-width: 100%to images - the max-width prevents it from overflowing its container.
Marked as helpful
@sthefanycaires
Posted
Hello @Islandstone89! Thank you so much for your feedback! I made some changes to the code following the tips you mentioned, but I removed some elements from CSS Reset that I wasn't using and created an <article> tag with the <h1> and <p> tags, because the distance between the image and the <h1> was different from the distance between <h1> and <p>. Your tips really helped me improve my code, thank you!
