Skip to content
Submitted 11 months ago

QR code component

amakura-411•60
@amakura-411
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

What are you most proud of, and what would you do differently next time?

当時はcssやclassの記法がまだまだわかっていなかったところなどがあるため、それらを変えていきたい。

What challenges did you encounter, and how did you overcome them?

cssの書き方ですね。

メンバーに意見をもらったりして、修正しました。

What specific areas of your project would you like help with?

モダンな書き方を知りたいと思っております。

例えば、クラス名の書き方、cssの読みやすい描きからなど

Community feedback

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

    Hi, here is some feedback.

    HTML:

    • Good job on including the <main>, most beginners are not aware of its importance. However, you have way too many divs. In such a simple component, you only need a <main>, which can also act as the card. So, I would remove all divs.

    • The image must have alt text. This is essential for screen readers to understand the image. The alt text should be descriptive, and in this example, it also needs to say where it leads (frontendmentor.io).

    • "Scan the QR code" and the footer text should both be wrapped in a <p>.

    • Hence, you should end up with the following HTML structure:

    | <body>
||   <main>
|||     <img>
        <h1>
        <p>
||   </main>
||  < footer>
|||     <p>
||   </footer>

    CSS:

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

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

    • Remove width: 100% on html, main and body - they are 100% wide by default, because they are block elements.

    • Change height: 100vh to min-height: 100vh. And you only need to put it on the body.

    • Background color and text alignment should be moved from the card to the body. Additionally, add Flexbox to center the card horizontally and vertically:

    background-color: hsl(212, 45%, 89%);
text-align: center:
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
min-height: 100vh;

    • max-width on the card is way too big, and should not be in px but in rem. 1440px equals to 90 rem, but around 20rem should be all you need.

    • Remove width: min-content.

    • Font-size must never be in px. Use rem instead.

    • Remove the fixed width and height on the image. You rarely want to set fixed dimensions, as this easily creates issues with responsiveness.

    • Do add display: block and max-width: 100% on the image. The max-width prevents the image from overflowing the card.

    Marked as helpful
    1
    amakura-411•60
    @amakura-411Posted 11 months ago

    @Islandstone89

    Thank you for the clear feedback, I appreciate it!

    I've improved the code based on your advice.

    However, there's one thing I'm not clear about.

    You suggested using max-width in rem instead of px. You mentioned that 1440px equals 90rem, but recommended using 20rem. How did you determine max-width: 20rem?

    I want to know how you arrived at that. (It might be adjustments based on actual screen view, but...)

    I'd appreciate it if you could provide an answer.

    Thank you in advance for your feedback!

    1
    Øystein Håberg•10,330
    @Islandstone89Posted 11 months ago

    @amakura-411 Happy to help :)

    If you open DevTools (click F12 in Chrome), you can select "Responsive" in the list of simulated devices. Then, you can adjust the screen size smoothly, and see where the card starts to get too big.

    Marked as helpful
    1
    amakura-411•60
    @amakura-411Posted 11 months ago

    @Islandstone89

    Thank you for your response!

    Thanks to you, it looks like I'll be able to write some good code!!

    I'm grateful to have met someone as kind as you! :))

    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