Responsive QR Code Component using flex box and proportion

Community feedback

• 

  Estifanos Wendimu•30

  @Steeve26Posted over 1 year ago

  Hey there 👋🏾. Great work! But it seems you haven't figured it all out so I'll shoot you some pointers. Also, feel free to check out my solution to the problem. It's got the proper HTML structure and efficient CSS to go with it.

  let's start with your Html. Instead of section, I suggest you use the HTML tag <main></main> to hold the main content and another container for the whole QR code card as well as a container for the two text elements (refer to my solution). I also suggest you use classes for styling your HTML instead of ID.

  Now about your CSS. I've noticed that you use all four properties of margin and padding separately a lot. It is much easier to just put them in one line like so: .mainBlock{ margin: 10px 4px 1px 12px} this means 10 top 4 right 1 bottom 12 right margin. You can also do this: "margin: 10px 12px" which means 10 top and bottom 12 left and right. And the same goes for padding.

  Something else I've noticed is you using percentage values a lot. It isn't usually necessary to use percentage values unless you don't know exactly how much the value of something should be (like when you want the qr code card thing to be 95% of the screen width for small devices which is what I did.) It can be a bad idea like how you used it for the border radius values which makes it a little wonky. always use exact values for that (3px - 10px is usually enough)

  And finally, about centering content. You need to get yourself familiar with flexbox as it will be your go to method for centering 90% of the time. All you have to do is "display: flex; align-items: center; Justify-content: center;" on the container elements (the 'main' container in this case, which you replaced with 'section') And their content will be centereď on both axes. Though keep in might displaying flex will arrange child elements horizontally. which you can counter with 'flex-direction: coulmn'.

  And one more thing, What you did with the media query is apply that CSS to all devices under 800px but the content is so small that It won't need adjusting until you get to the under 400px range. So if you're looking to target phones, go for under 500px. Hope this clarifies things ✌

  0
• 

  Ecem Gokdogan•9,380

  @ecemgoPosted over 1 year ago

  Some recommendations regarding your code that could be of interest to you.

  • In order to fix the accessibility issue, you need to replace <section class="mainBlock"> with the <main> tag. :)
  • You'd better use Semantic HTML, and you can also reach more information about it from Semantic HTML and Using Semantic HTML Tags Correctly.

  Hope I am helpful. :)

  0
• 

  P

  visualdennis•8,375

  @visualdennissPosted over 1 year ago

  Hey there,

  nice job. Here are some quick suggestions:

  Remove all of these from the .mainBlock:
  /* display: flex; / / flex-direction: column; / / margin-top: 12.5%; / / margin-bottom: 12.5%; / / margin-left: 40%; / / margin-right: 40%; */

  none of them are necessary. By default, it display:block works like flex-direction:column anw.

  • in general when one finds himself trying to calculate exactly percentages in paddings or margins to center elements, that is a sign of one is stepping into 'hackaround' territory, so it should indicate the fragileness of the structure, meaning a better solutions/practice should be sought.

  Here in this case just add these to the body { min-height: 100vh; display: grid; place-items: center; }

  and remove /* overflow: hidden; */ as it is unnecessary too. Using Flexbox or CSS Grid is much more stable and consistent way to center things.

  Hope you find this feedback helpful!

  0