Used CSS variables and BEM method with a bit Flex-box for aligningment

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 27 days ago

  Hi Danny, good job!

  Here are some suggestions :)

  HTML:

  • Remove the <section>, it is not needed. You only need the <div> holding the card content inside the <main> element.

  • You should not include words like "image" or "photo" in the alt text. Screen readers start announcing images with "image", so an alt text of "image of qr code" would be read like this: "image, image of qr code". 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."

  • I would change the heading to 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.

  • Wrap the footer text in a <p>.

  CSS:

  • Including a CSS Reset at the top is good practice.

  • "Outfit" is a sans-serif font, so change --ff-primary: "Outfit", serif; to --ff-primary: "Outfit", sans-serif;.

  • I recommend adding a bit of padding, for example 16px, on the body, to ensure the card doesn't touch the edges on small screens.

  • Move font-family and font-size from html to body.

  • On body, remove font-weight: 400 and overflow: hidden, as none are needed.

  • Remove all styles on .qr-code as the <section> is removed.

  • Remove the margin on the card.

  • To center the card horizontally and vertically, with some space between the <main> and the <footer>, I would use Flexbox on the body:

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

  • Remove width: 100% on the card - a <div> is a block element, meaning it takes up the full width by default.

  • max-width on the card should be in rem. Around 20rem 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.

  • letter-spacing must also not be in px. You can use em, where 1em equals the element's font size. Whenever the value is zero, you don't need to include the unit, so you would write letter-spacing: 0.

  • 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 setting max-width: 100% and height auto in images! It's also common to apply display: block - its default value of display: inline might cause unwanted space underneath an image if wrapped in a <div>.

  Marked as helpful

  1

  ---

  

  Danny-Agyei•10

  @Danny-AgyeiPosted 26 days ago

  @Islandstone89 : Thanks for the feedback. I'll surely look into that and update the code.

  1