QR code component

Community feedback

• 

  Sophia Koulen•150

  @sophiakoulenPosted over 2 years ago

  The code looks very nice to me!

  I'm not sure about using <h3> for the text, since you haven't used any <h1> or <h2> on the page and you shouldn't use tags that have a meaning for decorational purposes instead of adding extra css.

  And the margin: -50% doesn't seem to have any effect.

  You don't necessarily need to put your <img> inside a <div>. The <img> has by default a display value of inline, so it won't respond to width or height properties, but you can change that behaviour with 'display: block;' so it will act just like a <div>.

  The combination of absolute position and translation seems quite nice to me, i will definitely keep that in mind as a way to center an element. An alternative way, if your body has only one child, is to give 'height:100vh', 'display:grid', 'place-items:center' and the child is centered!

  0

  ---

  

  Luis•20

  @lnaranjocPosted over 2 years ago

  @sophiakoulen Thank you! I will keep in mind your advice :)

  0
• 

  LiBee•370

  @Li-BeePosted over 2 years ago

  Well done - close to the design. To fix the accessibility issues you need to add a <main></main> landmark

  <main>
      <div class="container">
          ....
      </div>
   </main>
  

  0

  ---

  

  Luis•20

  @lnaranjocPosted over 2 years ago

  @Li-Bee Thank you very much for your feedback. I am looking into landmark elements now, as I had never used them before (these are my first months of web developing). I will correct the code in the repository, and will take it into account for the future. Thank you very much.

  0

  

  LiBee•370

  @Li-BeePosted over 2 years ago

  @lnaranjoc same here! I only realised they were a thing a couple of weeks ago. 😁

  0