QR-Code Challenge

Community feedback

• 

  Vanza Setia•27,715

  @vanzasetia

  Posted almost 3 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. 😊

  06

  Marked as helpful
• 

  Ahmed Bayoumi•6,700

  @Bayoumi-dev

  Posted almost 3 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👍

  01

  Marked as helpful
• 

  Vanessa Méndez•30

  @vanmendez

  Posted almost 3 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. 😉

  01

  Marked as helpful