QR code component challenge written in HTML and SCSS

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hello there 👋. Good job on completing the challenge !

  I have some suggestions about your code that might interest you.

  HTML 📄:

  • Use the <main> tag to wrap all the main content of the page instead of the <section> tag. With this semantic element you can improve the accessibility of your page.

  • Since this component involves scanning the QR code, the image is not a decoration, so it must have an alt attribute. The alt attribute should explain its purpose. e.g. QR code to frontendmentor.io

  CSS 🎨:

  • Instead of using pixels in font-size, use relative units like em or rem. The font-size in absolute units like pixels does not scale with the user's browser settings. This can cause accessibility issues for users who have set their browser to use a larger font size. You can read more about this here 📘.

  I hope you find it useful! 😄 Above all, the solution you submitted is great!

  Happy coding!

  Marked as helpful

  2

  ---

  

  Dominik Gartz•140

  @domieeePosted almost 2 years ago

  @MelvinAguilar 👋Hi Melvin

  I just forgot to add the alt-tag, wops!

  You are of course right with the main-element and the font-size and i have already changed that.

  Thank's for your review!

  Have a nice evening, and stay coding!

  0
• 

  Leon Pahole•480

  @leonpaholePosted almost 2 years ago

  Hello Dominik!

  I took a quick glance at the code and here are my comments for imrpovement:

  • Your font-family declaration in css has no fallback font. Check this blog post for why this is important and for more information.
  • Your css selectors are very broad, like targeting the article tag. If you happened to have two articles on the page, they would both be styled by this declaration, which may or may not be intended. I suggest you instead use classes to target your elements. See this blog post
  • The container has width of 350px. This means that it is not scalable. If you resize the screen to below 350px, you will see horizontal scroll bar appearing. Instead, it might be better to use max-width with width set to 100%.
  • The container has height set to 530px. Setting fixed heights is very dangerous, because if you later change the size of the image, font size, text contents of the container or if the user manually changes their font size, it will cause the content to overflow the container. Instead, it would be better to just not set the height and let the content and the paddings dictate the height of the element.
  • Some font sizes are specified in px - I suggest you specify them all in rem. This great blog post explains why.
  • The folder structure is a little unconventional. The README file is in the assets/md folder, so it is difficult to find. README should ideally sit in the root folder, so it is automatically displayed when you open the repo in Github (unless it pertains only to a specific folder). Also, built code should probably not be commited (by built I mean css that is compiled from sass).
  • It is not apparent on how to run the project, how to run the sass compilation, etc. I suggest you look into Webpack, which will streamline the development process and document it so that anyone can easily run the project with a single command, rather than having to manually compile sass.

  Other than that, the code looks good to me and the component looks close to the design.

  Marked as helpful

  1

  ---

  

  Dominik Gartz•140

  @domieeePosted almost 2 years ago

  @leonpahole 👋Hi Leon,

  Especially the point of max-width has helped me. There I still have my problems. Also on Webpack I will take a special look, since I was not known before. The other things I will edit aswell. Thanks a lot!

  Have a nice evening, and stay coding!

  0