QrCodeComponent

Community feedback

• 

  David•8,000

  @DavidMorgadePosted about 2 years ago

  Hello Stalin, great job finishing the component challenge! congratulations!

  To answer your question, you can remove the header, its not a bad practice because this challenge doesn't even need a header!, also, html validators don't have a specific validation for header so you are good to go, putting it as an empty tag with nothing would really be a bad practice!

  Apart from that I have checked your project, I would do a couple of changes:

  Instead of using place-items: center try using place-content: center to fully center all of your grid items!

  If you want to fast position your footer, you can even use position: absolute, bottom: 1rem; align-self: center, position: absolute has no much good use cause, but this can be one of them!

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

  Marked as helpful

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  Hello Stalin,

  Congratulation on completing this frontend mentor challenge. Your solution looks great. I have some suggestions regarding your solution:

  • In this challenge, as it’s supposed to be a part of a whole page, you should use a header for class="image-title". Page should contain one level heading. In this challenge , as it’s supposed to be a part of a whole page, you may use<h1> instead of <p>.

  • In my opinion, the image is an important content. The alternate text is needed on this image. The alternate text should indicate where the Qr code navigate the user : like QR code to frontend mentor not describes the image.

  • I would not use <section> for just a part a the component. According to MDN , <section> is a generic sectioning element that is used whenever it doesn’t make sense to use the other more semantic ones.** It is a container that stores self-contained content that still makes sense when placed in a different context.**If you are only using the element as a styling wrapper, use a <div>.

  • There are a lot the arguments against the 62.5% font size trick ,it state that you should never change the root font size because it harms accessibility.

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

  hopefully this feedback helps.

  0