@Islandstone89
Posted
Hi. It looks good, but there are quite a lot of things to improve in the code.
HTML:
-
You need a
<main>. Change the first<div>to a main. And make the ID a class. -
qr-codeshould also be a class. IDs are rarely used these days. -
Alt text also needs to tell where it leads (frontendmentor.io).
-
"Improve your frontend skills" is a
<h1>. -
.attributionshould be a<footer>. And the text needs to be wrapped in a<p>.
CSS:
-
Don't use
@import. It's better for performance to link the fonts in the<head>of the HTML. -
It's good practice to include a CSS Reset at the top. Andy Bell has a good one you can use.
-
Remove
max-widthandmax-heighton thebody. Addmin-height: 100vh -
Change
justify-contentfromspace-betweentocenter, to center the card vertically. -
Remove
flex-wrap: wrapon the container. You do not need it here. -
Remove
box-sizing: content-box. It's the default, so you wouldn't have to declare it. Anyway, in the mentioned CSS Reset, the first thing it does is select all elements and appliesbox-sizing: border-box. -
Remove all fixed widths. Add
max-widthin rem on the card. -
Put
text-align: center;on the body. The text will elements will inherit the value. -
Font-size should not be in px. Use rem.
Marked as helpful
@0x-UG
Posted
@Islandstone89 thanks for this, I'm still new to it all so I don't necessarily know all the tips and tricks of CSS, I kinda tend to just do things manually like a noob would, I'll try these out right now!
@0x-UG
Posted
@Islandstone89 I'm done. Thanks for the feedback. Please be sure to give more feedback on my solutions whenever you can/see it. It'd be much appreciated. :) Thanks again!
