@Islandstone89
Posted
Hello, good job!
Looking through your code, these are my suggestions to improve your solution even further. I hope you find this feedback clear and helpful :)
HTML:
-
Every webpage needs a
<main>
that wraps all of the content, except for<header>
andfooter>
. This is vital for accessibility, as it helps screen readers identify a page's "main" section. Wrap the card in a<main>
. -
The alt text must also say where it leads(the frontendmentor website). A good alt text would be "QR code leading to the Frontend Mentor website."
-
Headings should always be in order, so you never start with a
<h3>
. I would make it a<h2>
- a page should only have one<h1>
, reserved for the main heading. As this is a card heading, it would likely not be the main heading on a page with several components. -
.attribution
should be a<footer>
, and you should use<p>
for the text inside.
CSS:
-
Including a CSS Reset at the top is good practice.
-
I like to add
1rem
ofpadding
on thebody
, to ensure the card doesn't touch the edges on small screens. -
Remove the
height
inpx
on thebody
. Never set fixed heights on elements containing text! If the content in the body grew to be taller than580px
, it would cause overflow. The content should decide how tall the body is. The body is by default only as tall as its content. Since we want to center the card vertically, we do need to create vertical space to center it in. The way to do that is to setmin-height: 100svh
on thebody
- this means the body will take up at least the full viewport height, but since it's a min-height, it has also room to grow if needed. -
Remove the
width
inpx
on the card. It is not recommended to set fixed sizes inpx
, unless sometimes for small things like icons. Instead, we want our components to adapt to all the different kinds of screens it can be viewed on. -
We do want to limit the width on the card to an extent, so it doesn't stretch too wide on large screens. We do this by adding a max-width, which should be in rem. Add
max-width: 20rem
on the card. -
font-size
must never be in px. This is a big accessibility issue, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead. -
Since all of the text should be centered, you only need to set
text-align: center
on the body, and remove it elsewhere. The children will inherit the value. -
Well done for having
max-width: 100%
on the image! Images, by default, take up the width of their intrinsic size, so if you have a card that is20rem
wide (equals320px
) and the image is500px
wide, it would overflow out of the card. Settingmax-width: 100%
makes the image shrink according to its container. Hence, it's best practice to set this on all images in every project you do. It's also common to adddisplay: block
on images, otherwise you might end up with a bit of unwanted space under images, as they are an inline element. -
Finally, I would probably slightly increase the
padding
on the card, to around16px
.
Keep up the good work!
Marked as helpful
@pudding-shark
Posted
@Islandstone89
Thank you for your response
It was very digestible (I am bit slow though so I might have missed something)
I made some adjustments based on your feedback. I also thank you for providing useful blogs/posts
I added you under "acknowledgements" in the README.md thing because you helped a lot. Hope you don't mind