QR code component using HTML and CSS

Community feedback

• 

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  👾Hello Kristina, congratulations on your first solution!👋 Welcome to the Frontend Mentor Coding Community!

  Nice code and nice solution! You did a good job here putting everything together. I’ve some suggestions for you:

  • Use <main> instead of a simple <div> this way you improve the semantics and accessibility showing which is the main block of content on this page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.
  • Use relative units like rem or em instead of px to have a better performance when your page content resizes on different screens and devices. REM and EM does not just apply to font size, but all sizes as well. To save your time you can code your whole page using px and then in the end use a VsCode plugin called px to rem to do the automatic conversion or use this website https://pixelsconverter.com/px-to-rem
  • Use a CSS reset to avoid all the problems you can have with the default CSS setup, removing all margins, and making the images easier to work, see the article below where you can copy and paste this CSS code cheatsheet: https://piccalil.li/blog/a-modern-css-reset/

  Here's my solution for this challenge if you wants to see how I build it: https://www.frontendmentor.io/solutions/qr-code-component-vanilla-cs-js-darklight-mode-nS2aOYYsJR

  ✌️ I hope this helps you and happy coding!

  Marked as helpful

  1

  ---

  

  Kristina Radosavljevic•380

  @KristinaRadosavljevicPosted about 2 years ago

  @correlucas Hi Lucas, thanks for your feedback! I didn't really focus on responsiveness for this challenge (hence the fixed measurements), although I probably should have given it a bit of thought at least. Your other comments were also very helpful (especially the one on resets - I always do just padding, margin and box-sizing, but there's apparently so much more, and I bet this link you provided will come in super handy especially for bigger projects), so this is definitely something I'll consider in my future challenges.

  Also, I looove your solution to this challenge, it's so much more than required, very nice job!

  Anyway, thanks again for your comments, I really appreciate it :)

  1

  

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  @KristinaRadosavljevic Thank you Kristina, keep posting amazing content =)

  1
• 

  cesar hernan ruscica•320

  @hernanruscicaPosted about 2 years ago

  Hi Kristina, I really like your code, because it is very simple. But I think you should use relative units, like EMS. Because I think, the challenge show us how it must to look in 375px mobile, but never say it has to be fixed. Other subject is that you don't use semantic HTML. But overall is great, and another think I like is that didn't use media queries! They aren't necessary in this project! Getting!!

  Marked as helpful

  0

  ---

  

  Kristina Radosavljevic•380

  @KristinaRadosavljevicPosted about 2 years ago

  @hernanruscica Hi there! Thanks so much for your feedback, I really appreciate it. And I think these are all valid points, I'll make sure to keep them in mind for the future challenges :)

  0
• 

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  Hi Kristina, congratulations on completing the challenge, great job! 😁

  Some little tips for optimizing your code:

  • add main tag and wrap the card for improve the Accessibility
  • also you can use article tag instead of a simple div to the container card for improve the Accessibility
  • remove all margin from .container class because with flex they are superfluous
  • use min-height: 100vh to body instead of height, otherwise the content is cut off when the browser height is less than the content
  • instead of using px use relative units of measurement like rem -> read here

  Hope this help! Happy coding 😉

  Marked as helpful

  0

  ---

  

  Kristina Radosavljevic•380

  @KristinaRadosavljevicPosted about 2 years ago

  @denielden Hi, Deniel! Thanks so much for your feedback, your points are really helpful :)

  I put margin on the .container because it has max-width, so when you shrink the screen, I don't want the edges of the box to 'stick' to the right and left edge of the screen, so I put some margin there to give it a bit of space. I don't know if this makes sense? :D

  Oh, and thank you for the tip on using min-height, I didn't even notice the weird thing that happens when you shrink the screen vertically, so this was definitely super helpful! :)

  1

  

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  @KristinaRadosavljevic You are welcome and keep it up :)

  Instead of the margin on the element it is better if you use side padding on the body in mobile resolutions. So if there are many elements you will not have to put many margins because the body will already have "reserved space" on the sides

  1

  

  Kristina Radosavljevic•380

  @KristinaRadosavljevicPosted about 2 years ago

  @denielden Right, that makes sense, it'll keep it in mind for next time :)

  1