QR Code Component

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 12 months ago

  Hey there, congratulations on doing this project. Here is my feedback.

  HTML:

  • You need a <main>, this is important for accessibility - change .container from <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, remove width: 100vw - the body is a block element, hence it is 100% wide by default. Also, change height to min-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-width of around 20rem on the card,to prevent it from getting too big on larger screens.

  • The image should not have a width, but a max-width: 100%, as well as display: 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: center on the body.

  Marked as helpful

  1

  ---

  

  P

  Darkstar•1,100

  @DarkstarXDDPosted 12 months ago

  @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.

  1

  

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 12 months ago

  @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.

  0