QR component using CSS and HTML

Community feedback

• 

  Lord Robins•550

  @ZenitsuAgPosted almost 2 years ago

  Hi MaltaWebDev, you've done an amazing job. Here are some tips to help you improve your code.

  • In the subheading class, it's better to add a h1 tag to wrap the text.
  • In the img tag, width=300 not 300px. But for responsive images in future, you can set the width to 100% or the max-width to 100% and it will scale down as automatically and it won't overflow the allocated space.
  • For the centering issue, you just add body {display: flex; justify-content: center; align-items: center;}
  • If the attribution div is causing you issues, you can just say position: absolute; bottom: 0

  So you may not really need to use CSS grid to center your code. Also, if you like the shadow, you can leave it but it's not really part of the original design.

  Happy Coding :)

  Marked as helpful

  1

  ---

  

  Edoye•170

  @DoyeDesignsPosted almost 2 years ago

  @ZenitsuAg can you explain why the width is to be 300 and not 300px

  0

  

  Gus•60

  @angusgeePosted almost 2 years ago

  @ZenitsuAg Thanks mate!!

  • h1 I missed, good spot. 🙌
  • thanks for this, you're absolutely right, no need for the "px" as per Google Lighthouse.
  • centring we can use either flex or grid here, thanks for confirming.
  • nice, I always wondered what position: absolute did haha now I know 🙌🙌

  Final thought, there's definitely a really subtle shadow on the design, it's visible on the snapshot... 😉

  Cheers!

  1

  

  Gus•60

  @angusgeePosted almost 2 years ago

  @DoyeDesigns it doesn't seem to be necessary. The HTML report you can generate when you submit the solution flagged that as an error too. 😉

  1
• 

  Edoye•170

  @DoyeDesignsPosted almost 2 years ago

  Good work Lord robins!

  You do not need css grid to center the page Elements. You can add;

  Body { Display: flex; Min-height: 100vh; Justify-content: center; Align-items: center; }

  You can check how I did mine. I reduced the amount of nested divs by grouping some elements into one div.

  2
• 

  Adriano•38,080

  @AdrianoEscarabotePosted almost 2 years ago

  Hi MaltaWebDev, how are you? I really liked the result of your project, but I have some tips that I think you will enjoy:

  To improve the responsiveness of the project, we can do this:

  .qr-code img {
      border-radius: 20px;
      width: 100% !important;
      max-width: 300px;
  }
  

  I noticed that you used inline css to put width in the img prefer not to do that, we consider it a bad practice because in larger projects, there may be some conflict between the values ​​that we put in the html and in the css, and changing these values ​​in different files takes a lot of time!

  The rest is great!

  I hope it helps... 👍

  1

  ---

  

  Gus•60

  @angusgeePosted almost 2 years ago

  @AdrianoEscarabote I'm fantastic thanks! Same to you brother. That's not inline CSS mate those are the height and width attibutes of the HTML element itself. Inline CSS looks like this, note the style tag <img style="width: 300px" > 😉

  0

  

  Adriano•38,080

  @AdrianoEscarabotePosted almost 2 years ago

  @MaltaWebDev if you want to keep it clean, you should never put a style attribute on any HTML tag. A CSS file should be used instead. Use an ID ou class instead. if you need to change something that's present on 50 HTML elements, you'll curse the day you did that!👍😉

  0