QR code component solution

Community feedback

• 

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  👾Hello Samuel, congratulations for your first solution and 😎 welcome to the Frontend Mentor Coding Community!

  You've done a really good work in your first solution! I've some tips for this and the next challenges:

  When you download the project files there’s a file called style-guide.md where you can find information such as hsl color codes and the font-size for the headings. The background-color in this case is background-color: #D5E1EF

  Look both width and max-width the main difference between these properties is that the first(width) is fixed and the second(max-width) is flexible, for example, a component with width: 320px will not grow or shrink because the size will be ever the same, but a container with max-width: 320px or min-width: 320px can grow or contract depending of the property you've set for the container. So if you want a responsive block element, never use width choose or min-width or max-width.

  Note that there's no need to use height here, because since you set a height for an element, this means that this element will grow until a certain point and after that the inner content (as texts or images) will start to pop out the element due its fixed height, so isn't necessary to set the height the container height comes from the elements, its paddings and height.

  A good practice to have all the image inside the container scaling and respecting the size of the container, you need to add max-width: 100% and add alsoobject-fit: cover to make the image auto-crop when resizing.

  img {
      display: block;
      object-fit: cover;
      max-width: 100%;
  }
  

  👋 I hope this helps you and happy coding!

  Marked as helpful

  0

  ---

  

  Samuel Jolayemi•190

  @SamjolasPosted about 2 years ago

  @correlucas Thank you for your feedback Lucas,

  I just made the necessary corrections to the code, you may check it out and see if all the feedback was well implemented.

  I am learning a lot, thanks to your feedback.

  Thanks.

  1

  

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  @Samjolas you're welcome Samuel, I'm happy ro hear that.

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  Hi Samuel Jolayemi,

  Congratulation on completing this challenge. Your solution looks great. I have some suggestions regarding your solution if you don’t mind:

  • Page should contain <h1>. In this challenge , as it’s supposed to be a part of a whole page, you may use<h1> visually hidden with .sr-only class.You can find here.

  • In my opinion, the alternate textis needed on this image. It should indicate where the Qr code navigate the user : like QR code to frontend mentor.

  • To center the component on the middle of the page you may use the flex/grid properties with min-height 100vh; to the body. Also you can add a little padding to the body to prevent the card from hitting. Then you can remove the position: absolute

  • width: 32.5rem an explicit width is not a good way to get a responsive layout. Consider using max-width to the card instead.

  • height: 50rem; It's not recommended to set height to component, let the content of the component define the height.

  • Add min-height: 100vh instead of height: 100vhto the body that let the body grows taller if the content outgrows the visible page instead.

  Aside these, Great work! Hopefully this feedback helps

  Marked as helpful

  0

  ---

  

  Samuel Jolayemi•190

  @SamjolasPosted about 2 years ago

  @PhoenixDev22 Thank you for your feedback PhoenixDev,

  I just made the necessary corrections to the code, you may check it out and see if all the feedback was well implemented.

  Also thanks for the .sr-only refresh page, I will take an in-depth study of it.

  I am learning a lot, thanks to your feedback.

  Thanks.

  0