Skip to content
Submitted 4 months ago

media queries and root from w3school

lawrence500•40
@lawrence500
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

  • Gwenaël Magnenat•1,270
    @gmagnenatPosted 4 months ago

    Hi, congrats on completing the challenge !

    I checked your repository and live url and have a few recommendations that you can use to improve your solution.

    Does the solution include semantic HTML?

    • you need to wrap your main content inside a <main> landmark. The main element represents the main content of the body of a document or application. -w3c
    • for increased performance you should place your title higher in the <head> just after the <meta name="viewport"..
    • when using a target="_blank" you should add a screen reader only indication that it will open in a new tab as it can be confusing for some users. But i'm not sure a clickable link is mandatory here.
    • you don't really need the extra div to wrap your <h2>
    • the 3 spans you added in the .bottom div can be just one <p> element with the full text.

    Is it accessible, and what improvements could be made?

    • Currently this solution isn't accessible for users who change their browser default font size.
    • You should change font size to relative units to make your text scale with user preferences Why font-size must NEVER be in pixels
    • You don't need the 80% width as it's only 80% of the screen size but doesn't scale with root font-size. Set a max-width instead only on the card and in relative unit (rem) so it can scale correctly.

    Does the layout look good on a range of screen sizes? Yes

    Is the code well-structured, readable, and reusable?

    • You need to add a modern css reset at the top of your stylesheet in all your project. It will help you with browser default styles and fix some issues before you start customizing. check this one for example modern css reset
    • you should focus on low specificity in your css before increasing it only when needed.
    • you can add more meaningful class name instead of .middle .top .bottom. For example .card__title, card__content. Imagine if we add 20 elements in this card, how would you name them if you are using top, middle, bottom?

    Does the solution differ considerably from the design?

    • The solution looks a bit bigger then the design. With a css reset it will help with proper font sizing and line-height.

    I hope you find something useful in these comments to improve this solution and your future challenges. Let' me know on discord if you have any question.

    Happy coding !

    0
    lawrence500•40
    @lawrence500Posted 4 months ago

    @gmagnenat thankyou for the feedback I will use this to improve my current solution also I was not aware that using px for font size is bad so thankyou for letting me know

    1
  • Nigusu Daniel•50
    @KingDan123Posted 4 months ago

    yes also use indentation. that's it.

    0
  • Nigusu Daniel•50
    @KingDan123Posted 4 months ago

    its good but make it well structured and readable

    0
    lawrence500•40
    @lawrence500Posted 4 months ago

    @KingDan123 when you say readable do you mean stuff like comments to

    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