Responsive QR Component using Flexbox

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi, Sourav Mehra! 👋

  Congratulations on completing your first Frontend Mentor challenge! 🎉

  Regarding your questions.

  • I would not recommend specifying any width and height on the card element. The card only needs a max-width to prevent it from becoming too large on wide screen sizes. For the height of the card, let the content inside it dictate how much height it should have.
  • In this case, you can finish this challenge without using a single div. In general, you want to use div as a container or a wrapper.

  Now, some feedback from me.

  • The QR code image doesn't need title attribute. The alt attribute is good enough for screenreader users to know what is the image. Also, I recommend improving the alternative text by telling the users the exact use case of the QR code. In this case, it is going to navigate the user to https://www.frontendmentor.io/. So, an alternative text like "QR code to frontendmentor.io" would be great.
  • I recommend using meaningful elements like h1 for the bold text and p element for the other text. It's best to always wrap text content with a meaningful element like a paragraph element whenever possible.

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

  Marked as helpful

  0

  ---

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @vanzasetia Thanks Vanza, just implemented your feedback. I had a doubt though. I want to be able to group the text section (the h and p elements) under one section just to be able to easily manipulate/style them if necessary (like changing their paddings/margins at just one place instead of doing it individually for each of the two). Do you think this is a good idea or correct thinking?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav Your approach is okay. But, I would rather add some padding on the card element to push all the elements inside the card. After that, I would add some margin-top and margin-bottom to the h1.

  This way, I don't have to use any div to create the spacing. I only need to add margin on the h1 to give some whitespace on the top and the bottom of it.

  By looking at your code, I would recommend:

  • Making all the elements as box-sizing: border-box. This will prevent the element from adding more width once you add more padding.
  • Setting the img element as a block element, max-width: 100%, and (maybe) height: auto. This will make it easier to work with img element.
  • Removing the margin from the .text element. I would recommend controlling the width of the elements inside the card with the padding of the card element instead.

  Next time, try to include those styling every time you create a new project. Think of it as the common CSS reset that would work on any project.

  Marked as helpful

  0

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @vanzasetia In the first iteration of this challenge, I didn't include the CSS reset (setting padding,margin as 0 for all elements), but did do so in the second iteration (which set all elements' margin,padding to 0). Is this the CSS reset you're talking about?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav The "CSS reset" that I was referring to is the img styling (make it as a block element...) and setting the box-sizing: border-box to all elements.

  The CSS reset that you mentioned can be the common CSS reset that you can apply every time you create a new project. 🙂

  0

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @vanzasetia For the last point (control width through padding), am I correct to assume that it's a good practice to have elements inside the card of the same width (img as well as .text div as 200px or so) and control the alignment of the .text div with its padding property?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav For the last point, I would recommend removing the .text element. As a result, the main.card element will be wrapping the img, h1, and p elements.

  After that, you can control the width of those elements with the padding of the .card element.

  As a side note, there's no need to set width to any elements inside the .card element.

  I hope this answer your question. 😅

  0

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @vanzasetia Just applied your latest feedback. I understand that it allows us to have more granular control over each text element (h1 and p here), but it also increases the number of places where the padding styles need to be changed if I decide to change them later. Any comments on this?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav I would recommend only applying the margin on the h1. It is because the h1 is between the img and the p elements. Also, there's no need for padding-right and padding-left on the h1 and the p elements.

  For the whitespace on the left and the right side of the h1 and p elements, I would prefer controlling them with the padding of the card element.

  By the way, here are the changes that I made with the developer tool.

  .card {
      background-color: var(--white);
      max-width: 22rem;
      border-radius: 1.4rem;
      /* padding: 1rem; */
      padding: 1.3rem;
     /* I increase the amount of padding to adjust the whitespace */
      box-shadow: 0px 20px 35px rgb(0 0 0 / 10%);
      margin-bottom: 30px;
  }
  
  .header-text {
      font-size: 1.5rem;
      color: var(--dark-blue);
      /* padding: 1.7rem 1rem 0 1rem; */
      text-align: center;
      margin: 1rem 0; 
      /* Just for the top and bottom, let the padding of the
          card element controls the whitespace on the right and left */
      /* I prefer using margin to give whitespace but,
           you can use padding if you want */
  }
  
  .description-text {
      /* padding: 0.35rem 1rem 1.35rem 1rem; */
      margin-top: 0.8rem; /* You can remove this margin if you want */
      font-weight: 400;
      font-size: 1.05rem;
      color: var(--grayish-blue);
      text-align: center;
  }
  
  /* Note: I recommend removing the commented code */
  

  So, regarding the issue, I can just change the margin of the h1 in case I want to have more whitespace between the p and the img elements.

  Marked as helpful

  0

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @vanzasetia I've applied your feedback. However, if I don't apply some more margin or padding on the p element, it doesn't look the same as the design ('the' appears on the 2nd line when no padding/margin is added). Is there any other way to handle this without adding margin/padding to <p>?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav This is the moment where you need to choose. So, do you want to choose maintainability or try to be pixel-perfect?

  I would choose maintainability. It is because if start adding styling just for the sake of being pixel-perfect then I might be ended up with a lot of hacks or unnecessary styling.

  Also, the design is just a guide. The code is the source of the truth.

  In general, you want to focus on the code quality instead. There's a blog post about pixel-perfect, written by Josh Comeau. I recommend reading it so you can understand that truly pixel-perfect is impossible.

  As long as your solution looks similar to the design. Then, it is already good. 🙂

  Marked as helpful

  0

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @vanzasetia Also, the design is just a guide. The code is the source of the truth.

  This is the first time I've got this crucial advice. Going to keep this in mind in my career, a fine addition to my collection. 😊

  Thanks a lot Vanza

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav No problem, Sourav! 👍

  0
• 

  Juan Bonilla•610

  @juanpb96Posted over 2 years ago

  Hey Sourav 👋

  Good job on this one!

  I think I can help you with your questions and include additional comments:

  • It is good to define width in your .class and you should change it according to what you want with media queries. I recommend starting with a mobile-first approach.
  • You can use different tags to provide a more sematic HTML structure in your code. For example, your <div class="attribution"> should be <footer class="attribution"> or <div class="card"> could be <main class="card">.

  I think it is not a personal preference in general. If you use tags properly, your site increases in accessibility and best practices. You can take a look at HTML reference as it has helped me when I wonder about tags.

  Hope this can be useful for you 😃

  Marked as helpful

  0

  ---

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @juanpb96 I would not recommend specifying the width to the card element. It will make the card not responsive because, on tiny screen sizes, the card is not allowed to shrink. Also, there's no need for media queries to make the site completely responsive. 😉

  I agree about the mobile-first approach. But, in this challenge, there's no need for media queries so I don't think it matters. 🙂

  Marked as helpful

  0

  

  Sourav Mehra•20

  @mehra-souravPosted over 2 years ago

  @juanpb96 Thanks Juan, just implemented this feedback. Can you confirm if screen readers can make do with landmark elements (main, footer, etc.) without including their respective roles (main, contentInfo)?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @mehra-sourav I can't confirm all screenreaders would treat those landmark elements the same way. So, I would recommend reading this blog post by Scott O'Hara where he gives some data about it.

  I can tell you that most of the time the semantic HTML is good enough. So, you might not need the role attribute if you already use the semantic HTML.

  1