QR code component - mobile first

Community feedback

• 

  T

  Gwenaël Magnenat•1,290

  @gmagnenatPosted 4 months ago

  Hi, congrats on completing the challenge !

  I noticed a few issues on your code that you may want to check. These are recommandations to improve your solution and good habits to take. Please check them out and reach out to me on discord if you have any doubt or questions about them.

  Does the solution include semantic HTML?

  • the minimal <main> landmark is here and properly used. You are also using the footer landmark for the attributions.
  • You could specify a better alt for the image. It should describe well what it is in case of this image cannot be seen. Here you can specify for example the target where this qr code will lead.
  • A performance recommandation : move the title in the head higher just under the <meta name=viewport.... There is great ressources about this shared on discord. You want the title of the page to displayed before everything else when a page is loading.

  Is it accessible, and what improvements could be made?

  • Currently you have an accessibility issue mostly caused by improper use of font-sizes, fixed width and height set on elements containing text. Users with visual impairement or low vision caused by disabilities or just aging will increase the default font size of the browser. If you are using pixels for your font sizes you set the size at a fixed value that will not respect the user settings and will not change accordingly. You need to use relative units for everything related to fonts (rem).
  • If you set a fixed width in pixels it's the same issue. The content will not scale if the font-size is increased so it will cause overflow and your layout will break. Use min-width in rem.
  • Don't set any fixed height on element that are containing text. Leave the content flow and scale naturally. If you set a fixed height you will again have overflow problem and a bad user experience for some people.

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

  • In addition to the point listed above, you have some useless rules in your stylesheet.
  • Remove the height on qr-box
  • Remove the height on qr__img and remove the width. It is contained in qr-box. just use a padding on qr-box
  • remove the width on qr__title and qr__undertitle. It's also contained in qr-box so you don't need to specfiy a width. You can limit the width of the text using the ch unit if you want.
  • remove the large padding-top on the body and use flexbox on your body to center the content.

  Does the solution differ considerably from the design?

  • the live url does not work.

  I hope you find some useful comments in this list. Sorry for the long list but I like commenting this challenge as I think its a perfect step to identify some pitfalls in fundamentals before moving to more complex projects. Reach out to me on discord if you have any questions about the list.

  Happy Coding !

  Marked as helpful

  0
• 

  Krushna Sinnarkar•1,100

  @krushnasinnarkarPosted 4 months ago

  Hi @KlaudiaK0205,

  Your HTML structure and class naming are mostly aligned with the BEM (Block Element Modifier) methodology, but there are a few adjustments and best practices you can consider:

  1. Block Naming: You've used qr-box for your main block, which is good. In BEM, blocks are standalone entities on a page that can be reused.

  2. Element Naming: For elements within the block, you've used qr__img, qr__title, and qr__undertitle. This is correct; elements are children of the block and are named with two underscores (__).

  3. Modifier Naming: BEM uses modifiers to change the appearance or behavior of blocks or elements. For example, if you had different styles for a smaller QR code image, you could use a modifier like qr__img--small.

  4. Avoid Styling Directly in HTML: Your current `` is fine for quick prototyping or small projects. For larger projects, it's better to move these styles to a separate CSS file like main.css, which you're already linking to.

  5. Class Naming Best Practices:

     • Use meaningful names that describe the purpose of the block or element.
     • Avoid nesting BEM naming too deeply (e.g., qr__title__text), as it can make your CSS selectors complex.

  Overall, your use of BEM methodology is good. As you continue to practice, you'll refine your naming conventions and structure further.

  One more suggestion i would like to give is that your component is not centered vertically. To ensure the qr-box is properly centered both vertically and horizontally on the screen add the following CSS rules to center the qr-box within the body:

  body {
      display: flex;
      justify-content: center; /* Centers horizontally */
      align-items: center; /* Centers vertically */
      height: 100vh; /* Ensures full viewport height */
      margin: 0; /* Reset default margin */
  }
  
  .qr-box {
      text-align: center; /* Center content within qr-box */
  }
  

  By applying these CSS adjustments, your qr-box should be properly centered on the screen regardless of the screen size, providing a clean and centered layout for your QR code component.

  Feel free to reach out if you have more questions or need further assistance. I hope you find my feedback valuable, and I would appreciate it greatly if you could mark my comment as helpful if it was!

  Marked as helpful

  0

  ---

  

  T

  Gwenaël Magnenat•1,290

  @gmagnenatPosted 4 months ago

  @krushnasinnarkar good feedback here :) I would just mention to use min-height: 100vh on the body instead of the fixed height. It will take the full viewport height but also allow body to scale if for example the default font-size is updated by the user to a larger value.

  Marked as helpful

  0