QR code component

Community feedback

• 

  jdrodriguez2707•170

  @jdrodriguez2707Posted about 1 month ago

  Hi, @oLucasstein. Hope you're doing very well. Your solution is very good, congratulations! 🎉

  Here are a few things I think could improve it even further:

  • Semantic HTML: Since we’re building a reusable component, wrapping all the content inside an <article> tag would be a great choice. This tag is meant for self-contained content, making the card more adaptable. Plus, you could replace the <h1> with an <h2>, assuming the card will be used within a page that already has an <h1>. This is just my perspective, thinking about how the component might be integrated into a larger project, but feel free to keep the <h1> if it makes more sense in your case.

  • Accesibility: You could try testing your site with different browser font sizes to see if anything breaks. I’d recommend using 'rem' instead of 'px' for those elements that need to adapt to user preferences, especially text. This helps ensure that your design remains accessible and adjusts properly to different user settings.

  • Code structure: You should use classes instead of ids so you can reuse styles when necessary. Maybe in this small project we can't see a difference but we'll do in larger projects. So it's good to get used to it, in my opinion. And last but not least, you could also use CSS custom properties to store those repeated values like colors, font sizes, spacing, etc. This makes your styles more maintainable and flexible. For example:

  :root {
    /* Colors */
    --slate-300: hsl(212, 45%, 89%);
    --slate-500: hsl(216, 15%, 48%);
    --slate-900: hsl(218, 44%, 22%);
    --white: hsl(0, 0%, 100%);
  
    /* Typography */
    --primary-font-family: 'Outfit', serif;
    --primary-font-size: 2.2rem;
    --secondary-font-size: 1.5rem;
  }
  

  Overall, you did it very well and solved the challenge, that's the most important thing. This is just my personal opinion and recommendations that I can give you based on my own experience. Keep up the great work and happy coding! 🚀

  Marked as helpful

  1

  ---

  

  oLucasstein•30

  @oLucassteinPosted about 1 month ago

  @jdrodriguez2707 Hi, thanks for the review. When you say to use an <article> tag you mean change div or just wrap it in main? I didn't knew about this properties and rem stuff, gonna do a research and update the project. That's all ty

  0

  

  jdrodriguez2707•170

  @jdrodriguez2707Posted about 1 month ago

  @oLucasstein You can wrap the two 'div' elements that you already have inside an <article> tag and then place it inside main, like this:

  <main>
      <article>   
         <div id="image-container">
            <img src="images/image-qr-code.png" alt="QRcode" class="image">
         </div>
         <div id="content">
            <h1 id="title">Improve your front-end skills by building projects</h1>
            <p id="sub">Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p>
         </div>
      </article> 
  </main>
  

  About the rem unit and acessibility stuff, I recommend this blog post, it explains why choosing the right unit is so important and some tips: https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/

  Let me know if you have any questions, best regards!

  0
• 

  Carrinson•10

  @CarrinsonPosted about 1 month ago

  it looks really good and has good responsiveness

  0