QR-Code Challenge

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi, John! 👋

  Congratulations on completing your first Frontend Mentor challenge! 🎉

  You've gotten some awesome feedback especially on solving the issues that have been reported. I have some feedback as well.

  • The alternative text of the QR code is not giving enough information. If you try a screenreader and then close your eyes, then the screenreader pronounces the image as "Frontend Mentor, image.". Then, what do you think the screenreader users would think about the image?
  • Remove all br elements from the paragraph. Let lines wrap where they need to. Screen readers will read out br elements. You can't accommodate every screen size, so it's rare you'll ever need to use them.
  • Remove width and height from the body. Never use 100vw on anything as it doesn't account for scrollbars when present. It may only ever introduce potential overflow/scroll bugs.

  That's it! I hope this helps. 😊

  Marked as helpful

  0

  ---

  

  P

  John•20

  @HeavyMetalCoffeePosted over 2 years ago

  @vanzasetia Thank you for your feedback!! Excellent, I didn't even stop to think about the little information I provided for the alternate text for the QR code. I always understood the less, most descriptive phrase as possible. I believe I have taken that just a bit too far...lol. As for the <br> tags, I did not know this, which means I need to align my paragraphs better. You make a great point about 100vw. The last challenge I did, the 3-Column preview, I ran into such an issue. Thank you again!!

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @HeavyMetalCoffee You're welcome! 😉

  So, what do you think is a better alternative text for the QR code?

  0

  

  P

  John•20

  @HeavyMetalCoffeePosted over 2 years ago

  @vanzasetia A descriptive text such as "QR code to visit Frontend Mentor".

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @HeavyMetalCoffee That's a great alternative text! So, why don't you update the alternative text on the live site? Also, if possible fix all the issues.

  Improving your code or fixing bugs is one of the skills that you will need as a developer. So, it's better to get used to it. 🙂

  0

  

  P

  John•20

  @HeavyMetalCoffeePosted over 2 years ago

  @vanzasetia I just updated and uploaded!! Thank you, I appreciate the feedback. Check it out!!

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @HeavyMetalCoffee I took a look at the updated solution. One thing, that I would like to suggest is to swap the header with div instead. header is a landmark element that is used to wrap certain page content like navigation, logo, etc.

  Other than that, everything looks good to me! 🙌

  0
• 

  Ahmed Bayoumi•6,740

  @Bayoumi-devPosted over 2 years ago

  Hey John, It looks good!... Here are some suggestions:

  • Document should have one main landmark, Contain the component with <main>.

  <main class="container">
     <div class="qr_code_card">
        //...
     </div>
  </main>
  

  • Page should contain a level-one heading, Change <p class="p1"> to <h1 class="heading"> You should always have one h1 per page of the document... in this challenge, you will use h1 just to avoid the accessibility issue that appears in the challenge report... but don't use h1 on small components <h1> should represent the main heading for the whole page, and for the best practice use only one <h1> per page.

  • You have used grid to center the component on the page, but you need to set the height of the container ---> min-height: 100vh; to center it at this height:

  .container {
     display: grid;
      justify-content: center;
      align-items: center;
      /* width: 1440px;        /* <---- Remove  */
      /* height: 800px;         /* <---- Remove  */
      min-height: 100vh;     /* <--- Add */
   }
  

  • Use REM for font size, It is a must for accessibility because px in some browsers doesn't resize when the browser settings are changed... See this article ---> CSS REM – What is REM in CSS?

  Hope this help!... Keep coding👍

  Marked as helpful

  0

  ---

  

  P

  John•20

  @HeavyMetalCoffeePosted over 2 years ago

  @Bayoumi-dev Thank you for the feedback!! I am currently stuck in the rut of watching tutorials and I am busting out that with these challenges. My next challenge, my goal is to use semantic tags properly after reading some of the feedback, especially using <h1>. Thank you for sharing the link to CSS REM, I appreciate it.

  0
• 

  Vanessa Méndez•30

  @vanmendezPosted over 2 years ago

  Good job!! 👏 I have a couple of observations, if you don't mind:

  • Use semantic tags instead of div's, for example the container can be a <main>
  • Each web page must have an <h1> tag; if you decide to add more <h?> make sure to follow the ascending order of these I am starting in web development, in the tutorials that I have seen they indicate that we must avoid these errors. 😉

  Marked as helpful

  0

  ---

  

  P

  John•20

  @HeavyMetalCoffeePosted over 2 years ago

  @vanmendez Thank you for the feedback!! Regarding the semantic tags is a great suggestion and with my next challenge I will incorporate that into my html. The use of the <h1> tag probably would have solved some layout issues. We watch so many tutorials and learn so many different ways to do things I forget some of the little things that impact greatly. Thank you again!!

  0