QR code component with CSS

Community feedback

• 

  Nguyen Nguyen•340

  @jesuisbienbienPosted over 2 years ago

  Hi Arjun,

  I took some time to look at your codes. Nicely done! I'd suggest you use display: flex to center your component. What I did was: 1- I removed the margin and margin-top in your container 2- I added this in the body:

  body {
   height: 100vh;
    width: 100vw;
    display: flex;
    justify-content: center;
    align-items: center;
    flex-direction: column;
  }
  

  Now things should be centered vertically and horizontally.

  I also recommend that you add some padding on both sides of the header and paragraph. Also, make sure to wrap the component in <main> tag.

  Hope this helps. Happy coding!

  Marked as helpful

  1

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @jesuisbienbien I don't recommend using height. It causes an issue where on small screen sizes, especially on mobile landscape view, the users won't be able to scroll to see the rest of the page content (the vertical scrollbar won't show up). So, my recommendation is to use min-height instead.

  For width: 100vw, there's no need to specify the width to make the card center horizontally. Also, the body element by default has full width and it can cause an issue where there's a horizontal scrollbar showing up. So, I highly suggest don't specify any width to the body element.

  1

  

  John Mirage•1,590

  @john-miragePosted over 2 years ago

  @jesuisbienbien I agree with @vanzasetia, always use min-height for this use case. Also:

  1. to center the card you can use margins.
  2. to let the card breathe add some vertical padding.

  .page {
    display: flex;
    min-height: 100vh;
    padding-top: 48px;
    padding-bottom: 48px;
  }
  
  .card {
    margin: auto;
  }
  

  2

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ChaosDynamix I've just tested it myself that setting margin: auto only makes the element horizontally center but, not vertically center. So, my recommendation is to just use flexbox to center the card.

  0

  

  John Mirage•1,590

  @john-miragePosted over 2 years ago

  @vanzasetia Are you sure you have the parent container as flex ?? My advice was that you dont need the following properties, but offcourse you still need flex display.

  justify-content: center;
  align-items: center;
  flex-direction: column;
  

  i just tested on a pen, and it works.

  CodePen

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ChaosDynamix Thanks for the clarification! Yes, I did the test myself without the display: flex. 😅

  0

  

  Nguyen Nguyen•340

  @jesuisbienbienPosted over 2 years ago

  @vanzasetia Thank you Vanza. I myself didn't know this. I learned something new today thanks to you.

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @jesuisbienbien No problem! 👍

  0
• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hello, Arjun! 👋

  Congratulations on completing your first Frontend Mentor challenge! 🎉 It's great that you are able to finish this challenge without a media query! 👏

  However, there are some areas that can be improved.

  • I recommend using margin instead br for spacing. br element has a meaning and it would be read out by the screen reader as "break". So, use CSS for styling.
  • You can improve the alternative text for the QR code by telling what is the QR code for. So, the better alternative text would be "QR code for frontendmentor.io".
  • Always use classes to reference all the elements that you want to style. Using id is going to make your stylesheet have high specificity (hard to maintain).
  • Consistency is important for your code. I know that it's a very tiny thing, <div id='container'> uses single quote where the other part of the HTML uses a double quote. So, I recommend changing it to a double quote instead.

  That's it! Hope you find this useful! 😁

  Marked as helpful

  0