@Islandstone89
Posted
Hey there, congratulations on doing this project. Here is my feedback.
HTML:
-
You need a
<main>, this is important for accessibility - change.containerfrom<div>to<main>. -
In this example, the alt text also needs to say where it leads (frontendmentor.io).
-
"Scan the QR code" is a paragraph, not a heading. Make it a
<p>.
CSS:
-
It's good practice to include a CSS Reset at the top.
-
On
body, removewidth: 100vw- thebodyis a block element, hence it is 100% wide by default. Also, changeheighttomin-height. -
Remove the fixed width and height on the card. You rarely want to set fixed dimensions, as this easily creates issues with responsiveness.
-
Add a
max-widthof around20remon the card,to prevent it from getting too big on larger screens. -
The image should not have a
width, but amax-width: 100%, as well asdisplay: block. This is included in the mentioned CSS Reset. -
Since all of the text is center-aligned, it's most efficient to set
text-align: centeron the body.
Marked as helpful
@DarkstarXDD
Posted
@Islandstone89 Thank you very much for your detailed feedback.
After reading your comment i went on google and read more about "HTML best practices". Helped me a lot to identify some very basic things that i have been doing wrong. I wasn't aware of landmark elements and their use cases even though i had seen other people use them.
I updated my code according to them and other feedback you provided regarding CSS. Didn't want to submit it as a new solution, so i just updated this one with those changes.
Again, thanks for taking a look at my code and giving me feedback.
@Islandstone89
Posted
@DarkstarXDD glad to help.
Well done on implementing the feedback, that is very important. One thing, though - min-height on the card should not be 475px, as that is a fixed width. Let the content decide its height.
