QR Code Component challenge using HTML5 semantics and CSS flexbox

Community feedback

• 

  P

  Øystein Håberg•11,010

  @Islandstone89Posted 4 days ago

  Hi, well done!

  Looking through your code, these are my suggestions for improvement.

  I hope they are clear and helpful - good luck :)

  HTML:

  • Remove <section>, it is not needed.

  • Remove <header> - this is used for content that stays the same on several pages, like a logo and navigation bar you often see at the top of every webpage.

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

  • Do not use <br> to force text onto a new line. The text should flow naturally, and all styling, including space between elements, should be done in the CSS.

  • Wrap the footer text in a <p>.

  • The <footer> must be outside of the <main>, since they are both semantic landmarks.

  • It's best practice to give elements a class, and use that as a CSS selector.

  • Ideally, your HTML should look something like this:

  <main>
    <div class="card">
      <img src="images/image-qr-code.png" alt="QR code leading to Frontend Mentor" class="qr-image">
      <h2 class="heading">Improve your front-end skills by building projects</h2>
      <p class="text">Scan the QR code to visit Frontend Mentor and take your coding skills to the next level.</p>
    </div>
  </main>
  
  <footer class="attribution">
    <p>Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>. Coded by <a href="https://github.com/ghazibenattia">Ghazi Benattia</a></p>.
  </footer>
  

  CSS:

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

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

  • On the body, change height to min-height: 100svh - this way, the content will not get cut off if it grows beneath the viewport. Add flex-direction: column, so the footer is underneath the main and not to its side.

  • Remove all widths in %.

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

  • On the image, remove max-height. Add display: block, height: auto and change max-width to 100% - the max-width prevents it from overflowing its container. Without this, an image would overflow if its intrinsic size is wider than the container. max-width: 100% makes the image shrink to fit inside its container.

  Marked as helpful

  0

  ---

  

  Ghazi Benattia•50

  @ghazibenattiaPosted 4 days ago

  @Islandstone89 Thanks a lot for the feedback, I will work on improving the site this weekend.

  1

  

  mkalkandev•70

  @mkalkandevPosted 3 days ago

  @Islandstone89 Hello. The challenge is not mine, but your comment gives me an idea, thank you for that. I request you to enlighten me about my challenge, any shortcomings or mistakes I may have.

  https://www.frontendmentor.io/solutions/creating-a-blog-card-using-flexbox-P2jMxfFuBJ

  and

  https://www.frontendmentor.io/solutions/css-flex-kullanarak-qr-menu-rmlbMA0zpO

  and

  https://www.frontendmentor.io/solutions/simple-social-media-links-using-flexbox-and-semantic-tags-9aGVHMi-U0

  1

  

  P

  Øystein Håberg•11,010

  @Islandstone89Posted 3 days ago

  @mkalkandev Sure, I'll take a look tomorrow.

  0

  

  mkalkandev•70

  @mkalkandevPosted 3 days ago

  @Islandstone89 I'm curious about your thoughts. Thank you.

  1