Product-Preview Card Component Challenge

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hi @superuser2345 👋, good job completing this challenge! 🎉

  I have some suggestions you might consider to improve your code:

  • Another alternative to using a flex layout for two columns with the same size is to use a grid layout:

  section > div {
      display: grid;
      grid-template-columns: repeat(2, 1fr);
  }
  

  • If the solution does not have content in the <header>, <nav> or <footer> elements, it is not necessary to add those elements.

  • In this challenge, you should not use the background property to set the image because this image has semantic meaning. Use the CSS background property if the image is not part of the content.

  Above all, the project is done well👏. I hope those tips will help you! 👍

  Good job, and happy coding! 😁

  Marked as helpful

  1

  ---

  

  Sunil Dutt•20

  @superuser2345Posted almost 2 years ago

  @MelvinAguilar Thanks Melvin, it's so kind of you that you have taken the time to share your valuable input.

  1. I thought grid is kind of old now and flexbox is what needs urgency to get hands-on right now, so that's why I have used that. I'm a bit confused between the two.
  2. Yes, that's I thought too in my first challenge, but Frontend Team has reverted with the remarks to keep it in practice.
  3. I have used background property to get rid of using two different image tags and for them two more div tags. If it's ok or not.
     And really thanks for helping with those suggestions, I will read further and implement them too. Thanks Sunil Happy Coding U 2! :)

  1

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  @superuser2345 Hello again ! Thanks for sharing your comments.

  • The platform warns you to use semantic elements, but you are not required to use all of them and even more if they have no content. If you are suspicious, you can see my solution to this challenge, where I only use a single semantic element (<main>), and my report works perfectly.

  In addition, I noticed that your solution has no styles on screens larger than 1440px due to the media query

  • When an image is set as a background, screen readers ignore those elements, making people with vision problems unaware of its presence

  You can use a <picture> tag when you need to change an image in different viewports. Using this tag will prevent the browser from loading both images, saving bandwidth and preventing you to have two image

  <picture>
     <source media="(max-width: 375px)" srcset="./images/image-product-mobile.jpg">
     <img src="./images/image-product-desktop.jpg" alt="your_alt_text">
  </picture>
  

  Marked as helpful

  1