Responsive QR code component

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi! 👋

  Congratulations on completing your first challenge! 🎉

  The HTML is simple and nice. It's great that you use the landmark elements correctly. Also, you already have a good alternative text for the image. Good job! 👏

  Some suggestions from me.

  • I recommend improving the alternative text by giving the users more information about the QR code. In this case, the QR code contains a URL (https://www.frontendmentor.io/). So, the alternative text would be "QR code to frontendmentor.io".
  • I would prefer making the body element as the main container of the page instead of using main element. So, I would set the min-height: 100vh and move all the flex properties on the body element instead.
  • The container only needs a max-width to prevent the card from becoming too large on wide screen. By doing this, you can remove the width properties and the media query. As a result, the site is completely responsive without media query.
  • To prevent the card from touching the edges of the browser, I recommend adding some padding on the body element.

  I hope this helps! Keep up the good work! 👍

  Marked as helpful

  0

  ---

  

  Emilly Cavalcante•10

  @httpsemillyPosted over 2 years ago

  @vanzasetia Thanks for your feedback, it's really helpful. I'll try to improve this project and keep your advice in mind for future ones. :)

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @httpsemilly You're welcome! Glad you found it to be helpful! 👍

  0
• 

  Saul•1,260

  @xsaulPosted over 2 years ago

  Hello Emilly, it is an amazing job! Just a comment: if in your <main> you use a height of 100vh you wouldn't need to add a media query. But is just a tiny detail. Congrats!

  0

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @xsaul I would not recommend setting height: 100vh on the main element. The issue with this is on mobile landscape view the card usually needs more height. It means that the users can't scroll to see the content that gets cut off.

  So, my recommendation is to set min-height: 100vh on the main element instead. This way, the users can scroll if the page content needs more than 100vh.

  Marked as helpful

  2

  

  Saul•1,260

  @xsaulPosted over 2 years ago

  @vanzasetia Oh yeah I didn't think about that. Thanks for complement my answer!

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @xsaul No worries! 👍

  0