QR Code Component

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 2 years ago

  Hello

  There's good feedback above that you still need to action.

  In addition

  • you need to update the font link in the document head. You've copied the same font twice. The second one is correct I expect as it includes the explicit weights. But you've not included the necessary two additional links from google fonts that need to go above it. Go back to Google fonts and copy the whole snippet as they provide it after selecting the font(s) and weight(s) you need. Paste all of it in
  • You must use landmark elements. This has been raised by two other people above and is called out in your accessibility report. Change the divs to main and footer. You will need to do this on every single challenge, every webpage should,at the very least have a main element
  • you don't need the content -desc div at all. Not sure why that's there
  • Ideally the alt text on the image should also say where the QR code goes to (to FrontendMentor.io)
  • inside the footer attribution you need to add a meaningful element wrapping the text. Never have text content in a div or span alone. Use a paragraph tag here
  • don't try to center content with huge paddings or margins. Flex or grid properties along with min-height 100vh is how you center a component in the viewport
  • the card should not have an explicit width. This is an important principle,to learn early. As soon as you give containers widths they are not fully responsive. Instead, let it be 100% wide (as with the default for block elements like divs), but set a max-width (preferably in rem). This is a good practice as it let's elements shrink if they need to when they have less space
  • use a basic css reset at the very start of every stylesheet. This will make different browsers default styles consistent and do useful things like set img tags to display block (instead of inline). Josh comeau and Andy Bell both have good modern examples of css resets you can use
  • Never ever put font size in px. Use rem. Extremely important.
  • low level elements like headings and paragraphs etc should not have padding. Padding is for internal space, margin is for external. When you want to create space between elements like these, you should be using margin. Again it's an important principle to understand early. Read about padding and margin as they have some important differences.

  Only once you've updated this challenge should you move on to another. Apply all feedback from here on your product preview card too, and only then ask for feedback on it.

  Good luck

  Marked as helpful

  1

  ---

  

  Sagun Shrestha•160

  @SaguneoPosted almost 2 years ago

  @grace-snow

  Wow! Thank you for such an insightful comment. Taking notes, going to make changes to the project, and will apply them to my future projects.

  0

  

  Sagun Shrestha•160

  @SaguneoPosted almost 2 years ago

  @grace-snow

  Just updated the code based on your suggestions!

  0
• 

  Favour•2,160

  @Nadine-GreenPosted almost 2 years ago

  HEY SAGUN!

  Congratulations on completing your first challenge, it is nice, however, I immediately noticed that it was not vertically aligned, to quickly fix this, you will need to give the body a height of 100vh and then place it in the center, the code to use is body:{height:100vh; place-items: center;}

  Additionally, instead of using so many div, consider using semantics instead, semantics include elements like article section etc you can learn more here, this will prevent your report from having accessibility issues, an example of this would be your .attribution class which instead of a div, you could have given a footer instead.

  IF YOU FOUND THIS IN ANY WAY USEFUL, DON'T HESITATE TO MARK IT AS USEFUL :)

  HAPPY CODING

  1

  ---

  

  Sagun Shrestha•160

  @SaguneoPosted almost 2 years ago

  Hey, the vertical alignment completely flew over my head! Thank you so much for your feedback and the additional resources provided.

  0

  

  Favour•2,160

  @Nadine-GreenPosted almost 2 years ago

  @Saguneo

  Happy that I could be of help :)

  0
• 

  Hassia Issah•50,650

  @HassiaiPosted almost 2 years ago

  Replace <div class="container"> with the main tag and <div class="attribution"> with the footer tag to fix the accessibility issues.

  To center a content on a page, add min-height:100vh; display:flex; align-items-center: justify-content: center; to the body.

  Use rem or em as unit for the padding , margin, width and preferably rem for the font-size for more on this watch this https://youtu.be/N5wpD9Ov_To

  Hope am helpful HAPPY CODING

  1

  ---

  

  Sagun Shrestha•160

  @SaguneoPosted almost 2 years ago

  Thank you for your feedback, and I'll definitely be using this method on my project!

  0