Skip to content
Submitted almost 3 years ago

HTML CSS

Nasim•20
@nasimabhari
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

  • PhoenixDev22•16,950
    @PhoenixDev22Posted almost 3 years ago

    Greeting @nasimabhari ,

    I have few suggestions regarding your solution:

    • There should be two landmark components as children of the body element - a main (which will be the component ) and a footer (which will be the attribution).<Footer> should not be in the <main >. HTML5 landmark elements are used to improve navigation .

    • Page should contain a level-one heading . you can use <h1> with class="sr-only" (Hidden visually, but present for assistive tech).

    • Image smust have an aalt text.

    • The alternative text is needed on that image for assistive tech and SEO search engine optimization . So it needs to contain a description like Qr code for frontend mentor .

    • To center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> definitely not position: absolute like this:

    body{
display:flex;
align-items: center;
justify-content: center;
width: 100%; 
min-height: 100vh

    /* max-width: 1440px;/you don't need to do that as the body represents your viewport. You can have a wrapper div that will be the only child of the body containing all the other elements, you can then set the max-width on that container. Even though In this challenge , You don't need to set the max-width:1440px/ and remove these too from the { min-width: 375px; }

    • Usingmin-height: 100vhinstead ofheight: 100vh;allow the body to set a minimum height value based upon the full height of the viewport.This also allows the body to to grow taller if the content outgrows the visible page.

    • width: 230px;an explicit width is not a good way . consider using max-width to card instead.

    • height: 380px; 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 .

    • You should use em and rem units .Both em and rem are flexible, Using px won't allow the user to control the font size based on their needs.

    • It's recommended not to use px for font-size. you can use rem instead .

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

    Marked as helpful
    0
    Nasim•20
    @nasimabhariPosted almost 3 years ago

    Hey @PhoenixDev22 , thank you so much for this feedback. I'm sure this will help me a lot on the further challenges.

    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