QR Component (v3)

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,160

  @MelvinAguilarPosted about 2 years ago

  Hi @nnall 👋, good job completing this challenge, and welcome to the Frontend Mentor Community! 🎉

  I have some suggestions you might consider to improve your code:

  • Use the <main> tag to wrap all the main content in your solution instead of using <div class="component">.

  • To make alternative texts more worthwhile, add descriptive text to the alt attribute of the QR image to explain what the QR image does. Upon scanning the QR code, you will be redirected to the frontendmentor.io website, so an example of alternative text would be "QR code to frontendmentor.io". You can read more about alternative text here.

  • Instead of using pixels in font size, use relative units of measure like rem or em. The font size in absolute length units (px) does not allow users with limited vision to change the text size in some browsers. Reference.

  • The <br> tag is not a semantic tag, so you should not use it. Also, if a screen-reader reads the text, it will break the flow of reading at the line break tag, so it should not be used to add vertical spacing. There are only a few cases where it is necessary (for example, in a poem or an address), and it is possible to avoid them by applying padding and margin styles via CSS. More information here.

  I hope those tips will help you! 👍

  Good job, and happy coding! 😁

  2

  ---

  

  Nicholas Nall•140

  @nnallPosted about 2 years ago

  @MelvinAguilar

  Thank you!

  Making changes now!

  1
• 

  T

  Alex•2,010

  @AlexKMarshallPosted about 2 years ago

  Well done for submitting your first challenge. It looks good, there are just a few things that would be worth sorting out. It will be helpful to get into these good habits when first starting out, as they become much more important as projects get more complicated.

  • Never set width or height on the body. You've set both of them: width: 100vw; height: 100vh;. The width is unnecessary here. The body will always take up the full width of the viewport unless you make it do something else. In addition, 100vw doesn't take into account things like scrollbars or device notches, so will often cause overflow. As it is both unnecessary and occasionally dangerous, just remove it.
  • You will probably want a min-height: 100vh here in order to vertically center the card. But make sure it is min height, not just height. By using height you make content get cut off if the screen is shorter or the content longer than you planned. Min height avoids that problem
  • Don't use width anywhere really. You have set width: 340px on your main element here. But what if the available space is less than that? You'll get overflow. Instead, use max-width which will allow things to shrink if necessary. In general, never use width or height other than for very small things like fixed-sized icons.
  • Use a CSS reset. It's not super important for this challenge, but having at least box-sizing: border-box on all elements will make things more predictable. Here you've set it to inherit which isn't a particularly useful option.
  • Make sure to understand the difference between padding and margin. Padding is the space between the edges of a box and its content. Margin is the space outside an element. Here you need padding on the <main> element as it needs to push its content away from the sides rather than margin on the children. Margin can be used to create vertical space between the children, or if the space is equal you can use gap on the parent instead, it's simpler to maintain.
  • Don't set spacing in percent. You have things like margin: 4% - aside from generally avoiding margin, where you do need to use it, use a predictable unit like 1rem. Using percentages will make things much harder to reason about when many elements are interacting with each other.
  • You have unnecessary spans inside your heading. I suspect you've done that to force a line-break. Don't do that, let the lines break where they naturally do given the space they have available.
  • Your heading should be a <h1>. Headings must go in order like a table of contents.
  • You'll want your image to be display: block to avoid the line height issues caused by the default display: inline. This would generally form part of the CSS reset
  • font-size: 1.08rem seems an odd choice. 1rem would be fine (and is the default)
  • To reiterate, remove the width: 80% on things like the paragraph and the attribution. It's unnecessary, the spacing should be determined by the padding on the parent.
  • Install Prettier in your code editor and make sure it is set to format on save. There are a lot of formatting inconsistencies in the code. When your code gets longer and more complicated this will make it far more difficult to read, so let an automated tool solve this for you.

  1
• 

  Nicholas Nall•140

  @nnallPosted about 2 years ago

  Going through this now thank you for the detail

  Also I do actually have prettier installed and running, could you say more on the "formatting inconsistencies" part?

  0