Pixel-perfect QR code component (HTML, CSS)

Community feedback

• 

  T

  Grace•30,990

  @grace-snowPosted almost 3 years ago

  Pixel perfect is a really unhelpful phrase, and not what you should be holding onto as a goal. If you were a pro member, or in a real job, you’d have access to the design files and established sizes for things like headings etc, so not something you need to waste time on doing these challenges. Just aim to get it close, that’s fine!

  The most important things are well written quality code.

  This looks good! 👍 Just small improvements needed

  • aria label isn’t valid on a h1. Instead use an sr only class and include the text inside the heading element. Or, as this is clearly a demo project I think it’s fine to add the h1 visibly on the page
  • your BEM is close, but you’ve forgotten to use the block class of card on the article
  • line height should be unitless, never in px
  • letter spacing needs to be in em really. Again, px no good coz it can’t scale with the text of sizes/zoom are changed
  • never have a max height on elements containing other content. If I had a larger font size or a smaller device, that max height could break the solution for me

  I hope this helps.

  Marked as helpful

  2
• 

  Joshua•25

  @minimalsmPosted almost 3 years ago

  @grace-snow all changes implemented, thanks again :-)

  0
• 

  Joshua•25

  @minimalsmPosted almost 3 years ago

  aria label isn’t valid on a h1

  Realised that after I submitted. I was fooled by this highly upvoted StackOverflow post. I guess it seemed to the most a11y friendly option, but I'll refactor this to use a sr-only class.

  your BEM is close, but you’ve forgotten to use the block class of card on the article

  Yes! Wasn't sure on this (it was one of my questions). Any thoughts as to using / not using BEM when we're dealing with small components like this?

  line height should be unitless, never in px

  Noted, will refactor 😀!

  letter spacing needs to be in em really. Again, px no good coz it can’t scale with the text of sizes/zoom are changed

  Ah good point, thanks!

  never have a max height on elements containing other content. If I had a larger font size or a smaller device, that max height could break the solution for me

  Lessons learned! Seems stylistically this wasn't doing anything anyway 😅 (at least at 'normal' font-size/zoom)

  Thanks so much for your thorough feedback @grace-snow ❤️

  0

  ---

  

  T

  Grace•30,990

  @grace-snowPosted almost 3 years ago

  @minimalsm this is really good overall, I’m just being picky 😉

  I shouldn’t really use the word invalid about aria label on the h1. It’s just not best practice. Bots will ignore it (like search engine crawlers) and h1 is very important content on a page.

  The other downside of aria label is it’s not always consistently translated if people switch their language settings (better than it used to be, eg google translate will scan aria label now, but behaviour is still patchy)

  Marked as helpful

  1

  

  Joshua•25

  @minimalsmPosted almost 3 years ago

  @grace-snow picky is good! It's how we learn 😬

  "is it’s not always consistently translated"

  Ah, that's interesting. I'll take a dive into that another day. The site my team works on is translated into ~40 languages, I wonder how this affects us 🤔

  0