Skip to content
Submitted 10 months ago

QR code v1.00

Izemmouren Ilyes•20
@Ilyes0izmr
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

i hope you guys help me to improve the code :))

Community feedback

  • Øystein Håberg•10,390
    @Islandstone89Posted 10 months ago

    Hi, here is some additional feedback.

    HTML:

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

    • .attribution should be a <footer>, and its text must be wrapped in a <p>.

    CSS:

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

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

    • Add around 1rem of padding on the body, so the card doesn't touch the edges on small screens.

    • height should be min-height - this way, the content will not get cut off if it grows beneath the viewport. I like to set it to 100svh.

    • center is not a valid value for margin-top, so remove that. You don't need a margin, so remove that as well.

    • Instead of using margin-bottom on the card, you can add a gap of 1 or 2 rem on the body.

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

    • The image needs a max-width: 100% on its default styling, not just when the viewport is larger than 500px. It is also common to give images a display: block.

    • The card should not have a fixed width, only a max-width of around 20rem, to prevent it from getting too wide on larger screens.

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

    • Paragrpahs have a default font size of 1rem, so there is no need for that declaration.

    • As the design doesn't change, there is no need for any media queries. When you do need them, they should be in rem, not px.

    Marked as helpful
    2
    Izemmouren Ilyes•20
    @Ilyes0izmrPosted 10 months ago

    thnx i will try to change it based on ur advice @Islandstone89

    1
  • Daniel 🛸•44,250
    @danielmrz-devPosted 10 months ago

    Hello @Ilyes0izmr!

    Your solution looks great!

    I have a suggestion for improvement:

    • For semantic reasons, use <main> to wrap the main content instead of a <div>. The tag <div> has no semantic value.

    📌 This tag change does not impact your project visually and makes your HTML code more semantic, improving SEO optimization as well as the accessibility of your project.

    I hope it helps!

    Other than that, great job!

    Marked as helpful
    2
    Izemmouren Ilyes•20
    @Ilyes0izmrPosted 10 months ago

    hello @danielmrz-dev thank you so much for the suggestion I will try to fix it

    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