Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

QR-code component using HTML and CSS

Chyka 60

@CrownedTechie

Desktop design screenshot for the QR code component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


  1. can you provide feedback on the structure and organization of my code?
  2. are there areas where my code could be more efficient?
  3. did I effectively implement a responsive design?
  4. any resources or tips you'd recommend for improving my skills?

Community feedback

P

@Islandstone89

Posted

Hi, here is some feedback:

HTML:

  • You need to have a <main>, this is important for accessibility. Change .container from a div to a main.

  • Alt text also needs to say where it leads(frontendmentor.io).

  • .attribution should be a <footer>.

  • Footer text needs to be wrapped in a <p>.

CSS:

  • It's good practice to include a CSS Reset at the top.

  • On body, remove width: 100% - as body is a block element, it takes up 100% of the width by default.

  • Remove position: absolute, top, left, and transform - you don't need any of these in this challenge. And you're already centering the card using Grid.

  • To center the card vertically, you do need to add min-height: 100vh to the body. As mentioned, it is 100% wide by default, but it does not fill the viewport height by default - instead, it gets its height from its content. So, it doesn't have any available space to center the card in, unless we give it a min-height of 100vh.

  • Remove the fixed width and height on the card. You rarely want to set fixed dimensions! Remember, we want our pages to be responsive, meaning they can adapt to different screen sizes. Setting fixed dimensions prevents this, and causes issues like overflow.

  • Remove the width and height of the image. You only need to set display: block and max-width: 100%, to prevent the image from overflowing its container.

  • You can also remove the margin - you're using padding on the card to get the space between the image and the edge of the card.

  • Font-size must never be in px. Use rem instead.

  • All the text is centered, so put text-align: center on the body, and remove it elsewhere. The result will be the same, as the children will inherit the value. But you are declaring text-align only once, which is more efficient.

Hope this is helpful - good luck!

Marked as helpful

1

Chyka 60

@CrownedTechie

Posted

Thank you so much for this🧎🏾‍♀️🧎🏾‍♀️🧎🏾‍♀️.... you're highly appreciated🥺❤️. I didn't know it was necessary in HTML to have landmark elements About the responsiveness, what do you think? I didn't use a media query, is that bad? @Islandstone89

1
P

@Islandstone89

Posted

@CrownedTechie happy to help :)

Yes, landmarks are important for screen reader navigation.

This card should have the same design on all screens, so you don't need a media query on this challenge.

Marked as helpful

1

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord