QR code component

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  Hi Trvan Quan,

  Congratulation on completing this challenge. Your solution looks great. I have some suggestions regarding your solution if you don’t mind:

  • You should use <main> for the card and <footer>. HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology. Page should contain one level heading. In this challenge, you may use <h1> for class=”bold”

  • In my opinion, the alternate text should indicate where the Qr code navigate the user : like QR code to frontend mentor

  • Adding rel="noopener" or rel="noreferrer" to target="_blank" links. When you link to a page on another site using target=”_blank” attribute , you can expose your site to performance and security issues.

  • Add min-height: 100vh instead of height: 100vhto the body that let the body grows taller if the content outgrows the visible page instead.

  • width: 280px an explicit width is not a good way to have a responsive layout. Consider using max-width to the card in rem instead.

  • Consider using rem for font size .If your web content font sizes are set in absolute units, such as pixels, the user will not be able to re-size the text or control the font size based on their needs. Relative units “stretch” according to the screen size and/or user’s preferred font size, and work on a large range of devices.

  Hopefully this feedback helps.

  Marked as helpful

  1

  ---

  

  Trvan Quan•100

  @69kwan69Posted about 2 years ago

  @PhoenixDev22 Thank you! After seeing your suggestions I know I have much more to learn.

  0

  

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  @69kwan69

  Happy coding !

  0
• 

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  Hello Sonic 😁 congratulations for your solution!

  Here's some tips I have for you after seeing your code, first of all I've to say that you did a nice work wrapping with a clean code, regarding the improvements. You can improve the html markup replacing the div with main to improve the accessibility and semantics and replace the height in body with min-height: 100vh to make sure the body will é displaying always at least minimum 100% of the viewport height.

  body { 
       font-family: 'Outfit', sans-serif; 
       font-size: 15px; 
       background-color: hsl(212, 45%, 89%); 
       display: flex; 
       align-items: center; 
       justify-content: center; 
  
       Min-height: 100vh; 
   }
  

  Hope this helps, happy coding

  1

  ---

  

  Trvan Quan•100

  @69kwan69Posted about 2 years ago

  @correlucas Thank you! This fixed my problem when minimized tab didn't fully display the QR code card.

  0
• 

  David•8,000

  @DavidMorgadePosted about 2 years ago

  Hello Trvan congrats on getting the solution for the challenge! and welcome to the frontendmentor community!

  You component styles looks great, you managed to center it using flex-box, maybe yours is a bit little than the challenge one but that doesn't matter at all.

  For you html tho, you should use at least a main on your component since every webpage should use a main tag on the principal content, since this one is just the component, you can use a main to wrap it all instead of a div.

  Also would recommend you to use an h1 tag, like the main tag, every webpage should have an h1 tag, it will improve the accesibility and will rank higher con search engines (wich means that your page will be more visited!)

  This are just things that doesn't matter that much in such a little project, but take them as a reference for your future challenges!

  Hope my feedback helps you, if you have any questions, don't hesitate to ask!

  1

  ---

  

  Trvan Quan•100

  @69kwan69Posted about 2 years ago

  @DavidMorgade Thank you! I didn't really pay attention to semantic HTML until now!

  1