Skip to content
Submitted almost 2 years ago

The very first challenge, QR code component

Pavlo Ivanichenko•10
@pashaproton
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

  • I'm not good at HTML and CSS so the most difficulty was understanding is it the right solution or not
  • Everything
  • Yes, I do have questions about best practices and I would like to get feedback on how I can improve my code

Community feedback

  • Account deleted

    Hi there! Great job on completing your first challenge! These are my suggestions:

    -- Pay attention to your accessibility report and work on correcting those errors!

    -- Your alt='' information should be more descriptive for screen readers. Try using "QR code for frontendmentor.io" (I got the same advice when I first did this).

    -- Wrap your main content with a <main> </main> tag for accessibility concerns. You can read more about the <main> tag here: Main Tag

    For example:

    <!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
  </head>
  <body>
    <main>
      <section>
</section>
    </main>
  </body>
</html>

    -- Your fonts don't load correctly. You'll want to head over to https://fonts.google.com/specimen/Outfit and click on the weights you want -- in this case, the 400 and 700. Click the '+' next to them, copy the link on the right-hand side and paste it into the head of your HTML.

    For example:

    <!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <link rel="preconnect" href="https://fonts.googleapis.com" />
    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
    <link
      href="https://fonts.googleapis.com/css2?family=Outfit:wght@400;700&display=swap"
      rel="stylesheet"
    />
    <title>Document</title>
  </head>
  <body>
    <main>
      <section>
</section>
    </main>
  </body>
</html>

    -- You can remove the @font-face { font-family: 'Outfit'; src: url('https://fonts.google.com/specimen/Outfit'); }" in your css as a result of the above font linking. That format isn't really wrong, just need some different information. You can read more about web fonts here: Web Fonts

    -- You don't really need to create a variable for the font weight. You can simply use font-weight: 400 ; and font-weight: 700; it's accomplishing the same thing and it's easier to read at a glance.

    -- You also don't really need a variable for border-radius:

    -- Try using relative values for your font-size: instead of pixels. You can read about relative values here: Relative Values and Units

    -- Practicing being specific or adding appropriate classes or IDs to target just those items in your CSS. Your current code down below targets all h1 in your code and it's the same for your p. That's fine for now, but you may not want to target all of them when you have multiple ones.

    You could do this:

    section h1 {
  padding: 1rem 0 0;
  font-size: 22px;
  font-weight: var(--weight-bold);
}

    That way, you only target all h1's in your sections -- which is still broad, but better. Eventually, you'll want to be even more specific.

    For example:

    .example  h1 {
  padding: 1rem 0 0;
  font-size: 22px;
  font-weight: var(--weight-bold);
}

    In this case, give assigned the class of example to the section by doing this: `section class="example".

    That's it for now, hopefully you found some of it helpful!

    Marked as helpful
    1
    Pavlo Ivanichenko•10
    @pashaprotonPosted almost 2 years ago

    @GenuineMiyashita thanks for advices. I disagree regarding --border-radius variable because the value is used in multiple places. After refactoring of selectors it would be not usable anymore

    0
  • Mirza Monirul Alam•680
    @WebDevMirzaPosted almost 2 years ago

    Hi,

    I found an area where you can improve your code.

    • Put everything inside <section></section> to <main></main> to solve accessibility WARNING that you have now. main is used for the unique part of the site where any repeated sections such as nav, sidebar, footer are not allowed. For more info visit W3 School

    Apart from this, the rest of the part is great.👍

    Marked as helpful
    1
    Pavlo Ivanichenko•10
    @pashaprotonPosted almost 2 years ago

    @WebDevMirza, I just updated the code, thank you!

    0
  • Melvin Aguilar 🧑🏻‍💻•61,140
    @MelvinAguilarPosted almost 2 years ago

    Hello there 👋. Good job on completing the challenge !

    I have some suggestions about your code that might interest you.

    Alt text 📷:

    • The alt attribute should explain the purpose of the image. Uppon scanning the QR code, the user will be redirected to the frontendmentor.io website, so a better alt attribute would be QR code to frontendmentor.io

      If you want to learn more about the alt attribute, you can read this article. 📘.

    CSS 🎨:

    • The width: 100vw property in the body tag is not necessary. The body tag is a block element and it will take the full width of the page by default.

    • Using height: 100vh or height: calc(100vh - 1px); for the body element can cause problems with the layout of the page on smaller screens, such as in landscape view on a mobile device.

      On smaller screens, such as in landscape view on a mobile device, the height of the viewport may be less than the height of the content of the page. In this case, using height: 100vh for the body element will cause the content of the page to be hidden behind the body element.

      Here is an image of how it would look on a mobile device (taking into account the scroll): screencapture-pashaproton-github-io-qr-code-component

      To avoid this problem, it is generally recommended to use min-height: 100vh instead of height: 100vh for the body element. This will ensure that the content of the page is always visible.

    I hope you find it useful! 😄 Above all, the solution you submitted is great!

    Happy coding!

    Marked as helpful
    0
    Pavlo Ivanichenko•10
    @pashaprotonPosted almost 2 years ago

    @MelvinAguilar thank you!

    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