Skip to content
Submitted 9 months ago

Frontend-Mentor|QR-Code-Challenge

abdelrahman ahmed•60
@a-ahmed151
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

this is my first project using plain html and css. I was wondering if there is a simpler way to center the div and picture with out guessing their width.

Community feedback

  • Grace•31,230
    @grace-snowPosted 9 months ago

    I agree with all of the above, except place the component inside the main landmark, don't replace it with main. In a real project this would need to be a reusable component and that would sit somewhere inside main.

    Similarly, this type of component wouldn't have a h1. Because it's unlikely to ever be a page heading. It's a component that would sit on a page that already had a h1, so I recommend changing this to use h2.

    It's important you grasp the foundational principles from this though, like

    • importance of a css reset
    • how to write good alt text (there's a great post about this in the resources channel on discord)
    • why you should never set width or height on components , preferring no height and max width in rem instead.
    • block vs inline elements (why we set img elements to display block and max-width 100% in a reset - you wouldn't normally need to add this yourself as its part of any good reset already).
    Marked as helpful
    1
    abdelrahman ahmed•60
    @a-ahmed151Posted 9 months ago

    Thanks for your comment this was very helpful @grace-snow

    0
  • Øystein Håberg•10,550
    @Islandstone89Posted 9 months ago

    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 the "main" section of a page. Change .container to a <main>.

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

    • Remove the margin on the card.

    • To center the card horizontally and vertically, use Flexbox on the body:

    display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
min-height: 100svh;

    • Remove all widths and heights.

    • Add a max-width of around 20rem on the card, to prevent it from getting too wide on larger screens.

    • font-size must never be in px. This is a really 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.

    • Paragraphs have a default value of font-weight: 400, so there is no need to declare it.

    • Use px instead of % on border-radius.

    • Remove margin on the image. Add display: block and change width to max-width: 100% - the max-width prevents it from overflowing its container.

    Marked as helpful
    1
    abdelrahman ahmed•60
    @a-ahmed151Posted 9 months ago

    Thanks for your helpful comment. will try to implement them in my next project. @Islandstone89

    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