Skip to content
Submitted 11 months ago

QR code component

Gladys•40
@linggladys
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...QR code component
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Solution retrospective

My first challenge in Frontend Mentor. Please provide me feedback or critiques. 😁

Community feedback

  • Melvin Aguilar 🧑🏻‍💻•61,140
    @MelvinAguilarPosted 11 months ago

    Hello there 👋. Good job on completing the challenge !

    I have some suggestions about your code that might interest you.

    • Wrap the page's whole main content in the <main> tag.
    • To vertically center it on all screens, you should use a height so that display: flex can handle the centering. You can use min-height: 100vh in the .container selector to make it occupy the full height of the screen and then remove the defined margin margin: 70px 0;. This ensures a centered layout on all screens.

    I hope you find it useful! 😄 Above all, the solution you submitted is great!

    Happy coding!

    2
    Gladys•40
    @linggladysPosted 11 months ago

    @MelvinAguilar Thank you for the feedback. I have zero understanding in the min-height and max-height property at the moment. 😅

    0
  • Øystein Håberg•10,490
    @Islandstone89Posted 11 months ago

    Hi, here is some additional feedback.

    HTML:

    • I would remove all divs, as you don't need it in this challenge. All you need is a <main>, with an image, heading, and a paragraph inside. The heading should probably be a <h1> instead of a <h2>.

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

    • Personally, I would add .attribution to the <footer>, and as mentioned, delete the div.

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

    CSS:

    • Performance-wise, it's better to link fonts in the <head> of the HTML then using @import.

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

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

    • Since all text is centered, you only need to set text-align: center on the body:

    • Remove the fixed width on the card. You should never set fixed dimensions, except for sometimes on things like icons.

    • Remove the height on the image. Change width to max-width: 100% and add display: block.

    1
    Gladys•40
    @linggladysPosted 11 months ago

    @Islandstone89 I appreciate your feedback and I find the linked articles you provided useful.

    1

Please log in to post a comment

Log in with GitHub

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