QR code component

Community feedback

• 

  P

  Øystein Håberg•10,390

  @Islandstone89Posted 3 months ago

  Hello, good job!

  Looking through your code, these are my suggestions to improve your solution even further. I hope you find this feedback clear and helpful :)

  HTML:

  • Every webpage needs a <main> that wraps all of the content, except for <header> and footer>. This is vital for accessibility, as it helps screen readers identify a page's "main" section. Wrap the card in a <main>.

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

  • Headings should always be in order, so you never start with a <h3>. I would make it a <h2> - a page should only have one <h1>, reserved for the main heading. As this is a card heading, it would likely not be the main heading on a page with several components.

  • .attribution should be a <footer>, and you should use <p> for the text inside.

  CSS:

  • Including a CSS Reset at the top is good practice.

  • I like to add 1rem of padding on the body, to ensure the card doesn't touch the edges on small screens.

  • Remove the height in px on the body. Never set fixed heights on elements containing text! If the content in the body grew to be taller than 580px, it would cause overflow. The content should decide how tall the body is. The body is by default only as tall as its content. Since we want to center the card vertically, we do need to create vertical space to center it in. The way to do that is to set min-height: 100svh on the body - this means the body will take up at least the full viewport height, but since it's a min-height, it has also room to grow if needed.

  • Remove the width in px on the card. It is not recommended to set fixed sizes in px, unless sometimes for small things like icons. Instead, we want our components to adapt to all the different kinds of screens it can be viewed on.

  • We do want to limit the width on the card to an extent, so it doesn't stretch too wide on large screens. We do this by adding a max-width, which should be in rem. Add max-width: 20rem on the card.

  • font-size must never be in px. This is a big accessibility issue, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead.

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

  • Well done for having max-width: 100% on the image! Images, by default, take up the width of their intrinsic size, so if you have a card that is 20rem wide (equals 320px) and the image is 500px wide, it would overflow out of the card. Setting max-width: 100% makes the image shrink according to its container. Hence, it's best practice to set this on all images in every project you do. It's also common to add display: block on images, otherwise you might end up with a bit of unwanted space under images, as they are an inline element.

  • Finally, I would probably slightly increase the padding on the card, to around 16px.

  Keep up the good work!

  Marked as helpful

  1

  ---

  

  pudding-shark•30

  @pudding-sharkPosted 3 months ago

  @Islandstone89

  Thank you for your response

  It was very digestible (I am bit slow though so I might have missed something)

  I made some adjustments based on your feedback. I also thank you for providing useful blogs/posts

  I added you under "acknowledgements" in the README.md thing because you helped a lot. Hope you don't mind

  1
• 

  Rabel•190

  @RabelahmedPosted 3 months ago

  Congratulations on completing your project. min-height:100vh; on body element to center your container horizontally and vertically also instead of using h3 use h1 skipping level is not semantic html if you need smaller font just change font size hope this helps

  2

  ---

  

  pudding-shark•30

  @pudding-sharkPosted 3 months ago

  @Rabelahmed

  Thank you for your response

  I added you under "acknowledgments" in the README.md thing.

  Hope you don't mind

  0

  

  Rabel•190

  @RabelahmedPosted 3 months ago

  @pudding-shark thank you for doing that

  0
• 

  Propowershell•70

  @PropowershellPosted 3 months ago

  Hello Pudding-Shark, You're QR code component is looking good so far but there are just two things I wanted to point out.

  1. The "Improve" in the heading element seems to be in serif font, maybe you should place the import link (<link href='https://fonts.googleapis.com/css?family=Outfit' rel='stylesheet'>) in the head tag in the HTML file.

  2. Use Slate 900: hsl(218, 44%, 22%) for the heading element's color(it's recommended you use a h2 element since it's not the first heading tag in the page.

  Other than that it's great.

  0