QR code component using flexbox

Community feedback

• 

  turanarican•200

  @turanarican2022Posted 10 months ago

  First, congrats for completing the challenge. As per your questions;

  1. FlexBox is a good solution here as it is very suitable to use for component based layout,
  2. I used a .container div with a background-color of that faint blue to center the card in the viewport with a height of 100vh and width of 100vw and I used a .card div like you used a .wrapper div
  3. if you want to use rem units, make sure to set font-size: 62.5% in your root html element so that when you size something for 1.2rem for example it becomes 12px

  Hope these help.

  2

  ---

  

  P

  Øystein Håberg•10,390

  @Islandstone89Posted 10 months ago

  @turanarican2022 you should not change the font size to 62.5%. This article by Grace Snow, one of the top contributors here on Frontend Mentors, explains why.

  0
• 

  P

  Øystein Håberg•10,390

  @Islandstone89Posted 10 months ago

  HTML:

  • Every webpage needs a <main> that wraps all of the content, except for <header> and footer>. This is vital for accessibility, as it helps screen readers identify the "main" section of a page. In this simple component, it can also act as the card, as that's the only "main" content on the page. I would change .card into a <main>, and delete the .container <div>.

  • The image must have alt text. This is essential for screen readers to understand the image. The alt text should be descriptive, and in this example, it also needs to say where it leads (frontendmentor.io).

  • .attribution should be a <footer>.

  • Footer text needs to be wrapped in a <p>.

  CSS:

  • It's good practice to include a CSS Reset at the top.

  • Move all the properties you had on .container over to the body, except the width - block elements are 100% wide by default, so there is no need for that declaration. Also:

  • Change height to min-height - this way, the content will not get cut off if it grows beneath the viewport.

  • Since the body has two children, <main> and <footer>, you need to set flex-direction: column - the default flex-direction: row would make them side by side.

  • Finally, add a gap of a couple of rems to create space between them.

  • max-width on the card must be in rem. Around 20rem is suitable.

  • Remove the margin on the card.

  • font-size must never be in px. This is bad for accessibility, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead.

  • Since all text should be centered, you only need to set text-align: center on the body, and remove it elsewhere. The children will inherit the value.

  • Remove height, change width to max-width: 100%, and add display: block to the image.

  1

  ---

  

  Noel Hoppe•350

  @noelhoppePosted 10 months ago

  @Islandstone89

  Hi! First, thank you very much for your feedback. I tried to implement your suggestions but I have furthermore three more questions:

  1. Is it a good practice to use the px unit for margin and padding?
  2. To: "Since the body has two children, <main> and <footer>, you need to set flex-direction: column - the default flex-direction: row would make them side by side. Finally, add a gap of a couple of rems to create space between them." : The footer element has a position: absolute. That's why I don't have to use flex-direction: column and I don't need the gap property.
  3. To: "Remove the margin on the card." : I think I need the margin property on main because the main section has margin on the sides.

  Thanks a lot!

  1

  

  P

  Øystein Håberg•10,390

  @Islandstone89Posted 10 months ago

  @noelhoppe Good job on implementing the changes! Your code is greatly improved :)

  Regarding your questions...

  "Is it a good practice to use the px unit for margin and padding?"

  I have used mostly rem, and haven't thought about the implications when users change their default font size. I did a quick Google search just now, and though I will look more into it, I did find this interesting article. It is written by a Web Accessibility Specialist, so I trust she knows what she's talking about. And it does make sense to think of margin and padding as something that should not scale. So yeah, it seems using px for margin and padding is OK.

  "To: "Since the body has two children, <main> and <footer>, you need to set flex-direction: column - the default flex-direction: row would make them side by side. Finally, add a gap of a couple of rems to create space between them." : The footer element has a position: absolute. That's why I don't have to use flex-direction: column and I don't need the gap property."

  You are correct about that. Just be aware you'll probably need to do it in another challenge. Also, on my laptop screen, the card fills almost the whole vertical space of the viewport. Since the <footer> is positioned to the bottom, it actually overlaps the card. That's another thing to be cautious of.

  "To: "Remove the margin on the card." : I think I need the margin property on main because the main section has margin on the sides."

  Not quite sure what you're trying to say here. It is true that it can be useful to put a little bit of eighter: margin on the card, or maybe even better, padding on the body, so you don't risk the card squishing against the edge on smaller screens. But you certainly don't need any margin on the card to center it, when it is already centered using Flexbox.

  Marked as helpful

  1

  

  Noel Hoppe•350

  @noelhoppePosted 10 months ago

  @Islandstone89

  I tried to use your feedback to finish another challenge and I have some questions there. I would be happy to get some suggestions:

  I've just completed a front-end coding challenge from @frontendmentor! 🎉

  You can see my solution here: https://www.frontendmentor.io/solutions/blog-preview-card-using-flexbox-qotQmW9ky7

  Any suggestions on how I can improve are welcome!

  1