Responsive qr code card

Community feedback

• 

  Hassan Moataz•1,860

  @hassanmoaaPosted 9 months ago

  Hello @chilledoutluke!

  Great Job solving the challenge mate congrats 🎉

  Some suggestions for improvements.

  For the font-size it's is better to use rems and ems but px for this project is no big deal.

  font-size: 13px;

  • i see you using pixels for many elements, never use pixels for font-sizes in any element, here's why:

  • Certain font-related CSS properties will render your site completely inaccessible if their value is declared using pixels even once.

  Which properties are affected?

  All of these properties must never ever be declared in pixels:

  • font-size
  • line-height
  • letter-spacing

  If you've used pixels to define any of the above style properties, these will not respect the user's font size preferences!

  • You should use ems, and rems for font-sizes would be better

  This article may help:

  https://fedmentor.dev/posts/font-size-px/

  Other than that you're good, keep up the good work!

  Marked as helpful

  0

  ---

  

  Luke James Yengopal•100

  @chilledoutlukePosted 9 months ago

  @hassanmoaa Thanks for the great suggestion. I read the article, it was helpful and easy to understand. Will definitely use ems, rems and % instead of pixels in the future.

  0
• 

  P

  Øystein Håberg•10,570

  @Islandstone89Posted 9 months ago

  HTML:

  • Remove the <figure>, it is not needed.

  • The alt text must also say where it leads(frontendmentor.io).

  • Footer text should be wrapped in a <p>.

  CSS:

  • Add around 1rem of padding on the body, so the card doesn't touch the edges on small screens.

  • Remove the padding on main.

  On .container:

  • Remove min-width

  • Change max-width to rem, and make it much smaller. Around 20rem(equals to 320px) is suitable.

  • Remove the width in px.

  • Remove the margin

  • To center the card horizontally and vertically, use Flexbox on the body:

  display: flex;
  flex-direction: column;
  justify-content: center;
  align-items: center;
  min-height: 100svh;
  

  • Remove the margin on the image, and change width to max-width: 100%.

  • font-size must never be in px. This is bad for accessibility, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead. It should also not be in %.

  • Since all of the text should be centered, you only need to set text-align: center on the body, and remove it elsewhere. The children will inherit the value.

  • Paragraphs have a default value of font-weight: 400, so there is no need to declare it.

  Marked as helpful

  0

  ---

  

  Luke James Yengopal•100

  @chilledoutlukePosted 9 months ago

  @Islandstone89 Hey! Thanks for the really good suggestions. I went ahead and basically read up on ems and rems, I fully understand how to implement them into my projects and will do so for future projects.

  I went back to the project and made the necessary changes, it definitely makes a difference to the responsiveness of the QR component. I definitely have room for improvement but this really helped.

  1