Skip to content
Submitted over 2 years ago

HTML,CSS and media queries

Muhammad Asim Sajjad•80
@asimsajjad-dev
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

Community feedback

  • Travolgi 🍕•31,400
    @denieldenPosted over 2 years ago

    Hi Muhammad, I took some time to look at your solution and you did a great job!

    Also I have some tips for improving your code:

    • add main tag and wrap the card for Accessibility
    • remove all margin from .container class
    • try to use flexbox to the body for center the card. Read here -> best flex guide
    • after, add min-heigth: 100vh to body because Flexbox aligns child items to the size of the parent container

    Overall you did well :)

    Hope this help and happy coding!

    1
    Muhammad Asim Sajjad•80
    @asimsajjad-devPosted over 2 years ago

    @denielden thanks for you suggestion check now

    1
  • PhoenixDev22•16,950
    @PhoenixDev22Posted over 2 years ago

    Greeting @asimsajjad-dev ,

    Congratulation on completing your first Frontend mentor challenge,

    I have few suggestions regarding your solution:

    • After you fixed the accessibility issues , you may want to generate another report for your solution.

    • To center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> . Remove the margin from the container

    body {
    line-height: 1.5;
    /* width: 1440px; */
    min-height: 100vh;
    text-align: center;
    display: flex;
    align-items: center;
    justify-content: center;
}
.container {
..............
 /*   width:400px ;*/ Use ``max-width:20rem`` instead, let it grow to a point. Then a little margin on the component or padding on the body to stop it hitting screen edges.
 margin: 1rem;
    /* height: 650px; */ It's rarely ever a good practice to set heights on elements,you almost   never want to set it . let the content define the height . 
      ....................
    /* margin: 30% auto; */
}

    • remember a css reset on every project. That will do things like set the images to display block and make all browsers display elements the same.

    • I would not use a grid approach to align the items in the card or the extra divs.

    • It's recommended to use em and rem units .Both em and rem are relative units , Using px won't allow the user to control the font size based on their needs.

    Overall , your solution is good. Hopefully this feedback helps.

    0
    Muhammad Asim Sajjad•80
    @asimsajjad-devPosted over 2 years ago

    @PhoenixDev22 thanks for suggesting check my solution now

    0

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