QR Card Component

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 4 months ago

  Hi, good job!

  Here is some feedback - I hope you find it helpful :)

  HTML:

  • Every webpage needs a <main> that wraps all of the content, except for <header> and footer>. This is vital for accessibility, as it helps screen readers identify a page's "main" section. Change .container into 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."

  • "Improve your" should be a <h2>.

  CSS:

  • font-family should be placed on the body, not on *.

  • I like to add 1rem of padding on the body, to ensure the card doesn't touch the edges on small screens.

  • You don't need to set margin: 0 on the body when it's already declared on *.

  • Remove all positioning properties. A better way to center the card horizontally and vertically, is to use Flexbox on the body:

  display: flex;
  flex-direction: column;
  justify-content: center;
  align-items: center;
  min-height: 100svh;
  

  Be sure to replace height with min-height - this way, the content will not get cut off if it grows beneath the viewport.

  • div .qr-container and qr-container .image-qr-code are both descendant selectors, with added specificity. To keep the specificity flat, I would write .qr-container and .image-qr-code instead.

  • max-width on the card should be in rem. Around 20rem(equals 320px will work fine.

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

  • On the image, add display: block and change width to max-width: 100% - the max-width prevents it from overflowing its container.

  • Instead of setting padding on .image and .content, I would add padding on the card itself:padding: 16px. The increase from 10px gives the card a bit more "space".

  Marked as helpful

  0