QR code component challenge

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted almost 2 years ago

  Hi

  This looks smaller than the design and the component is cut off screen on my mobile. Hopefully these suggestions will help

  1. Remove width 100vw. Never use that anywhere, it would only ever cause overflow bugs with horizontal scroll. You don't need a width here anyway - the element is already 100% wide by default
  2. Change height 100vh to min-height. This is what is causing the bugs in my mobile view because you are limiting the height
  3. Use landmark elements. The card needs to be in a main element, and the attribution needs to be a footer
  4. You don't need as many divs as this. Try to make it as simple as possible
  5. No media queries needed on this challenge
  6. Never limit the height of elements that contain text. Let the browser decide how tall it needs to be
  7. The component should not have a width, only a max-width. So it can shrink if it needs to
  8. The style guide said what size body font should be, but converted into rem. This is how large paragraphs should be (your challenge is too small).
  9. The img element doesnt need srcset attribute or object fit. You should always include a modern css reset at the start of the stylesheet. Amongst other things that would set img elements to display block and max-width 100%
  10. The image is the most important content on this challenge. It needs a proper human readable description of what the image shows, and in this case what url it goes to

  Marked as helpful

  1

  ---

  

  s17•60

  @s17-gitPosted almost 2 years ago

  @grace-snow thank you for your feedback. I've made some changes to my project, could you take a look of it please.

  And tell me I'm use to wrapping all my content with a div like in vuejs app div#app, is it really necessary ?

  I'm also using a div for my component, is it fine or should I use another semantic HTML tag like section ?

  Thank you !

  0

  

  T

  Grace•31,230

  @grace-snowPosted almost 2 years ago

  @s17-git much better 👏 well done

  • you don't need to wrap everything in an extra div and you don't need any more semantic elements
  • you don't need the div wrapping card text content
  • you DO need to rewrite the image alt text still (point 10 in my first feedback)
  • you DO need to update the font size so it matches what it says in the style guide but converted to rem

  0

  

  s17•60

  @s17-gitPosted almost 2 years ago

  @grace-snow thank you one small.

  1st, 2nd, and 3rd point done !

  last point: the base font size is 15px and I converted it into rem which is 0.938rem.

  I'm using 1.1rem in my component title in order to be closer to the design.

  Do I really need this footer ? otherwise my markup will be much smaller .

  Eg:

  <main class="bee-qrcode-app"> <img alt="a QRcode to scan"> <h1 class="qrcode__title">Improve your front-end..</h1> <p class="qrcode__desc">Scan the QR code...</p> </main>

  0