QR Code Component

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 4 days ago

  Hi Daniel, good job!

  Here are some suggestions:

  HTML:

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

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

  • .attribution should be a <footer>, and you should use <p> for the text inside. The <footer> must be outside of the <main>. Your HTML should look like this:

  <main>
    <div class="card">
      <img src="./images/image-qr-code.png" alt="QR code leading to the Frontend Mentor website." />
      <div class="card-text">
        <h2>Improve your front-end skills by building projects</h2>
        <p>Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p>
      </div>
    </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="#">Daniel Knight</a>.</p>
  </footer>
  

  CSS:

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

  • You can remove font-family: "Outfit", sans-serif on the heading - since it is set on the body, all descendants will inherit the value.

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

  • Remove all widths and heights in px. We rarely want to give a component a fixed size, as we need it to grow and shrink according to the screen size.

  • We do want to limit the width of the card, so it doesn't get too wide on larger screens. To solve this issue, give the card a max-width of around 20rem.

  • 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. NB: whenever the value is zero, you don't need to include the unit.

  • 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, add display: block, height: auto and max-width: 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.

  • Remove position: absolute on the footer. Instad, add gap: 2rem on the body.

  Marked as helpful

  1

  ---

  

  Daniel Knight•20

  @DKulanPosted 3 days ago

  @Islandstone89 thank you for the great suggestions! I have applied most of them to my solution but did a couple things differently:

  1. Left the h1 element since the title within the card is the main heading on the page. I agree that if this was a reusable component it should be an h2 instead (and hopefully an h1 is set somewhere else on the page).
  2. That CSS reset kept the default top margin for the text elements, I used this one instead.
  3. Converted all font size + letter spacing properties to rem/em. Used the Calc() function cuz math iz hard. I left the footer gap in pixels because the gap in rem caused a lot of unnecessary spacing when scaling up the font size.
  4. didn't need to set the image height + display properties since the reset is doing it already.

  1

  

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 3 days ago

  @DKulan All good 🙂

  1
• 

  dolly-alt•10

  @dolly-altPosted 4 days ago

  I agree with your sentiment about cleaning up the CSS. As a newbie, it looks pretty good to me but I understand the need to have less clunky css for better readability.

  Your layout looks great to me though, and I've noticed some properties that I could have used on my solution.

  1