QR Code Challenge

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 24 days ago

  Excellent advice, Kamran!

  I'll add the following:

  HTML:

  • The alt text should be written naturally, without using - between the words. Write something short and descriptive, without including words like "image" or "photo". Screen readers start announcing images with "image", so an alt text of "image of qr code" would be read like this: "image, image of qr code". The alt text must also say where it leads(the frontendmentor website). A good alt text would be "QR code leading to the Frontend Mentor website."

  CSS:

  • I recommend adding a bit of padding, for example 16px, on the body, to ensure the card doesn't touch the edges on small screens.

  • Remove the margin on the card.

  • As mentioned above, setting fixed sizes is not ideal, so remove all widths and heights. Add a max-width of around 20rem on the card.

  • 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.

  • In addition to max-width: 100%, I also like to set height: auto and display: block on all images. Remove margin-top.

  • To create the space between the image and the edge of the card, set padding on all 4 sides of the card:padding: 16px;.

  Marked as helpful

  1
• 

  P

  Kamran Kiani•1,280

  @kaamiikPosted 24 days ago

  Hi. Congrats.

  • Your HTML structure is good. You have a main element in your html that is good. and You didn't create extra ‍div‍‍s. But this is not a page and you don't need h1 here. h1 is like a title of your page. Use h2 instead.

  ---

  • Try to use a proper CSS reset at the start of your CSS style. Andy Bell and Josh Comeau both have a good one. You can simply search on the internet to find them.

  ---

  • Use min-height: 100vh; instead of height:100vh;. height: 100vh strictly limits the height to the viewport size, potentially causing overflow issues if the content is larger than the viewport. On the other hand, min-height: 100vh allows your element to grow in height if the content exceeds the viewport size. Also put your min-height inside the body.

  ---

  • Never limit your width and height in a container or element or tag that contains text inside. When you limit the width and height of elements containing text, you risk the text being cut off, overflowing, or becoming unreadable, especially on smaller screens or when the text dynamically changes. It's generally better to allow the container to adjust its size based on its content or set a flexible size that can adapt to different screen sizes and text lengths. You only need max-width here in your .container because it prevents elements from stretching beyond a certain point, keeping them visually appealing across different screen sizes. It ensures your design remains adaptive and doesn't get too wide on larger screens.

  ---

  • For your img you can use width and height but in this challenge there is no need. You just need max-width: 100%; for your image. If you add a proper CSS reset to your style, then this is used for img.

  ---

  • Your font-size and max-width should be in rem unit not px. You can read this article about it and why you should not use px as a font-size.

  Marked as helpful

  1

  ---

  

  wrinklytech•10

  @WrinklytechPosted 23 days ago

  @kaamiik thanks for your feedback, much appreciated. Just one issue; moving the min-height setting to the body causes the flex items to move to the top of the screen.

  0

  

  P

  Kamran Kiani•1,280

  @kaamiikPosted 23 days ago

  Add the flex settings too. @Wrinklytech

  min-height:100vh;
  display: flex;
  flex-direction: column;
  justify-content: center;
  

  Marked as helpful

  1