qr-code-component-main

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted over 1 year ago

  Hello there 👋. Good job on completing the challenge !

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

  CSS 🎨:

  • You should use a CSS reset. A CSS reset is a set of CSS rules that are applied to a webpage in order to remove the default styling of different browsers.

  • You should use the box-sizing: border-box property to make the width and height properties include the padding and border of the element. This will make it easier to calculate the size of an element. You can read more about this here 📘.

    Your styles indicate a width of width: 300px; for the component, but if you inspect the element, the actual width is 332px, 32px more that are part of the padding and border.

  • Update the image selector to make responsive images.

    .card { /* NOTE: Only use the card selector here */
        border-radius: 0.75em;
        width: 300px;
    }
    
    img.qr-code {
        max-width: 100%;
        display: block;
    }
    

  I hope you find it useful! 😄

  Happy coding!

  Marked as helpful

  1
• 

  Maykol Vallejos•200

  @maykol-vallejosPosted over 1 year ago

  Hi, I was reviewing your code and I have some suggestions to make:

  -The first one is that you should use a CSS reset to remove the default styling of the different browsers. I think a previous comment mentioned it to you, for that you could use the following code:

  *{ margin: 0 padding: 0; box-sizing: border-box; }

  • Once the CSS is reset, you can center your card with flexbox or grid, in this case use grid to center it in your code, add the following code to your stylesheet and it will be centered for any responsive browser:

  body{ display: grid; place-items: center; min-height: 100vh; }

  -Lastly I would like to suggest you to try not to use "px", it is a bad practice when you are just starting in responsive web scope, especially with font size, for that I recommend you the 62.5% trick, you can check it in the following link:

  https://www.aleksandrhovhannisyan.com/blog/62-5-percent-font-size-trick/

  I hope the suggestion helps you, congratulations for your coding, keep it up, success in future projects!

  Marked as helpful

  0
• 

  Abdul Khaliq 🚀•72,560

  @0xabdulkhaliqPosted over 1 year ago

  Hello there 👋. Congratulations on successfully completing the challenge! 🎉

  • I have other recommendations regarding your code that I believe will be of great interest to you.

  HTML 🏷️:

  • This solution generates accessibility error reports due to non-semantic markup, which lack landmark for a webpage

  ---

  • So fix it by replacing the element <div class="card"> the with semantic element <main> along with <div class="attribution"> the with semantic element <footer> in your index.html file to improves accessibility and organization of your page.

  ---

  • What is meant by landmark ?, They used to define major sections of your page instead of relying on generic elements like <div> or <span>

  ---

  • They convey the structure of your page. For example, the <main> element should include all content directly related to the page's main idea, so there should only be one per page

  ---

  I hope you find this helpful 😄 Above all, the solution you submitted is great !

  Happy coding!

  Marked as helpful

  0