QR code component with self-hosted web fonts

Community feedback

• 

  Sabine•80

  @SabineEmdenPosted 4 months ago

  Kagan MacTane is a web developer based in Brooklyn and San Francisco. He was so kind to give me a very thorough code review after I posted a link to my solution on Mastodon. Here are Kagan’s comments and my response.

  Kagan: "I'm not sure the div inside the footer is necessary. You can just put the div's content directly into the footer tag if you want."

  Before:

  <footer>
    <div class="attribution">
      Challenge by
       <a href="https://www.frontendmentor.io?ref=challenge" target="_blank"
       >Frontend Mentor</a
       >. Coded by
       <a href="https://www.frontendmentor.io/profile/SabineEmden"
       >Sabine Emden</a
       >.
    </div>
  </footer>
  

  .attribution {
    font-size: 0.6875rem;
    text-align: center;
  }
  .attribution a {
    color: hsl(228, 45%, 44%);
  }
  

  I agree. The div inside the footer isn't necessary. It's there because it and the styling for its attribution class were part of the starter code. But there is no rule again replacing divs with semantic HTML tags. I'll remove the div and replace the class selector in the CSS with the element selector for the footer.

  After:

  <footer>
     Challenge by
     <a href="https://www.frontendmentor.io?ref=challenge" target="_blank"
       >Frontend Mentor</a
     >. Coded by
     <a href="https://www.frontendmentor.io/profile/SabineEmden"
       >Sabine Emden</a
     >.
  </footer>
  

  footer {
    font-size: 0.6875rem;
    text-align: center;
  }
  footer a {
    color: hsl(228, 45%, 44%);
  }
  

  Kagan: "You've got some gnarly fractions going on in your font sizes. Maybe you could just set the base font size to be something a little larger or smaller, and then your decimals might be more like .5, .75, etc.? (Not sure; I haven't done the math on it and it might not be feasible.)"

  Before:

  body {
    ...
    font-size: 0.9375rem;
    ...
  }
  
  .card-qr-code h2 {
    font-size: 1.375;
    ...
  }
  
  footer {
    font-size: 0.6875rem;
    ...
  }
  

  The font sizes are 15px (0.9375rem) for the paragraph, 22px (1.375rem) for the heading, and 11px (0.6875rem) for the footer. The fractions make the code less readable. It’s difficult to tell what the font sizes are without pulling out a calculator.

  I can think of four possible solutions:

  • Change the font sizes slightly to 1rem (16px), 1.5rem (24px), and 0.75rem (12px).
  • Use the The 62.5% Font Size Trick and set the root font size to 62.5%, which is equivalent to 10px. This changes the font sizes in the design to 1.5rem, 2.2rem, and 1.1rem.
  • Leave the font sizes in rem as they are and add comments with the sizes in pixel to the style sheet.
  • Use calc() to convert the font sizes.

  I’m a hesitant to change the font sizes. I got the values for the font sizes from the style guide and the Figma files. The brief for the project is to “get it looking as close to the design as possible”, and the font sizes are part of the design.

  I’ll go with the comments. They are the simplest option, stay true to the design, and will still work if the QR code component is used as part of a bigger project.

  After:

  body {
    ...
    /* 0.9375rem = 0.9375 * 16px = 15px */
    font-size: 0.9375rem;
    ...
  }
  
  .card-qr-code h2 {
    /* 1.375rem = 1.375 * 16px = 22px */
    font-size: 1.375rem;
    ...
  }
  
  footer {
    /* 0.6875rem = 0.6875 * 16px = 11px */
    font-size: 0.6875rem;
    ...
  }
  

  Kagan: "Also, I see you're using HSL colors. I'd like to know more about that decision (I'm more used to RGB). Were you advised to use HSL? Saw that in an article or course somewhere?"

  I used HSL colors because that’s how the colors were given in the style guide for the project.

  Kagan: "Also, just gotta say, and this is totally just my own pet peeve about spelling: there's a bunch of WOOF2 instead of WOFF2. It's just in documentation and obviously doesn't affect functionality at all, but it makes me twitch. Like I said, nitpicking!"

  I totally agree! That needs to be changed. Thanks for pointing it out to me! That does not show attention to detail.

  0