QR code challenge

Community feedback

• Account deleted

  Hey Dion, congratulations on completing the challenge! You did a great job 😉

  Let me give you some little tips for optimizing your code:

  • add main tag and wrap the card for improve the Accessibility
  • add descriptive text in the alt attribute of the images
  • centering a div with top isen't good practice, it uses modern css like flexbox or grid
  • remove all margin from .hero class
  • use flexbox to the body to center the card. Read here -> flexbox guide
  • after, add min-height: 100vh to body because Flexbox aligns child items to the size of the parent container
  • instead of using px use relative units of measurement like rem -> read here

  Hope this help! Happy coding 😁 by Travolgi

  1

  ---

  

  Dion•80

  @Dion-RPosted over 2 years ago

  @travolgi i've made the changes and the design feels allot better. cheers for the suggestion to use flex to align the card in the middle of the screen. Never thought of making the body a flex container and setting a min height until now.

  1

  Account deleted

  @Dion-R now it's much better! Great work keep it up :)

  0
• 

  Lucas 👾•104,340

  @correlucasPosted over 2 years ago

  👾Hello Dion, congratulations for your solution!

  The design elements and the html structure are all fine, but I've inspected your code and I've some suggestions for your: 1.Use a single padding to separate the container contents from the card bounds instead of inserting the padding for each element 2.Try to use h1 instead of h3 inside the card, start ever which h1 and keep the heading sequences. 3.Work better the text and image paddings, for example use padding-bottom to separate every element, I've fixed your code and will write you the correct padding for each heading. 4.Make your container flexible using max-width instead of width not that you component ins't stretching when the window get smaller due the fixed width set inside the container.

  See the code fixes below:

  .hero {
      max-width: 260px;
  }
  
  img {
      padding-bottom: 18px;
    }
  
  h1 {
      padding-bottom: 12px;
      /* margin: 10px 10px; */
      text-align: center;
  }
  
  p {
      text-align: center;
      /* margin: 10px 10px; */
     }
  
  

  Apply these changes and say me if works. Happy coding!

  1

  ---

  

  Dion•80

  @Dion-RPosted over 2 years ago

  @correlucas thanks for the feedback. Your suggestion to use padding helps allot as I was wondering whether to use margin or padding to seperate elements. I have a question, if i add padding to the bottom of the image to seperate it from the h1 element, will this impact the border radius of the image as it now has padding ?

  0