HTML and CSS for QR code component

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 12 months ago

  Hi, good work on the challenge! Among other things, you are using semantic HTML elements (<main> and <footer>), Custom Properties, and a proper CSS Reset.

  Here are a few tweaks that would improve your solution even further.

  HTML:

  • I'm not an accessibility expert, but I'm not sure if you should use aria-describedby in addition to having alt text.

  • Footer text needs to be wrapped in a <p>.

  CSS:

  • Add min-height: 100vh on the body to center the card vertically. The reason it is not centered vertically, even though you're using justify-content: center, is because the body by default is only as tall as its content, meaning there is no available vertical space to center the card in

  • Remove max-height on the card.

  • Remove the height and width on the image. You have already set the necessary max-width: 100% in the CSS Reset.

  • Instead of writing text-align: center on 3 different elements, you can put it on the body - this way, the elements will inherit the value, and you get the same result with less code.

  • Remove max-width on the footer.

  Marked as helpful

  1

  ---

  

  P

  Rupali•780

  @rupali317Posted 12 months ago

  Hello @Islandstone89

  Thank you for taking the time to review my code :D I have some thoughts/clarifications for some of the comments

  HTML

  • The reason I added the aria-described was that I wanted to form a connection between the subtext "Scan the QR code..." and the main QR image. This can be handy for blind users who can rely on assistive technology to understand the connection between those two.
  • Agreed! Using <p> tags helps convey the semantic structure of the document.

  CSS

  • The min-height:100vh that you suggested adding is already part of the CSS reset.
  • The suggestion related to removing max-height on the card, is it because it is redundant?
  • Initially, I did not have width and height on the image. I just had the dimension set in the CSS, as you mentioned. However, when I ran the page in PageSpeed, Insights, it required me to specify the width and height as part of the attributes to prevent CLS issues.
  • Agreed! Good catch!
  • The reason I added max-width on the footer is specifically for smaller screens. I noticed that in small screens, the entire text was hitting the vertical edges of the screen. In order to provide some breathing space in smaller screens, I added the max-width so that it at least wraps in two lines.

  Looking forward to your response. I appreciate your help

  1
• 

  Daniel 🛸•44,310

  @danielmrz-devPosted 12 months ago

  Hello @rupali317

  About your questions:

  • box-shadow and border-radius are parts of the project that are not likely to change. They are visual details and probably won't affect the user experience. So I thing it's acceptable using pixels.

  • You used padding correctly to handle the extra space. Since this project is the same both on mobile and desktop versions, it won't be a problem.

  I hope it helps!

  Great job!

  Marked as helpful

  1

  ---

  

  P

  Rupali•780

  @rupali317Posted 12 months ago

  Thank you for the explanation @danielmrz-dev :D

  2