Responsive Product Preview Card Component

Community feedback

• 

  Lucas 👾•104,400

  @correlucasPosted about 2 years ago

  👾Hello @youssefmagdy21, Congratulations on completing this challenge!

  Great code and great solution! I’ve few suggestions for you that you can consider adding to your code:

  1.I saw that you’ve used flexbox to place the content and create the layout.

  I think the best way to build this component with two columns is by using GRID LAYOUT since it is simpler to manage the columns and then create the media query for mobile. Here’s the steps to create it with grid create the main block to hold all the content (you can use <main> to wrap), set its width as max-width: 900px (it's the container size) and display: grid / grid-template-column: 1fr 1fr(this means that your component will have two columns with 50% of the container width each thats 450px). To manage the column with the text use flexbox and gap to give it the spacing between the texts or use padding-bottom to separate them.

  Then to create the mobile version, all you need to do is to change the container flow vertically with grid-template-column: 1fr.

  2.IMPROVE YOUR WORKFLOW using VSCODE you can code your whole page using px and then in the end use a plugin called px to rem here's the link → https://marketplace.visualstudio.com/items?itemName=sainoba.px-to-rem to do the automatic conversion or use this website https://pixelsconverter.com/px-to-rem

  ✌️ I hope this helps you and happy coding!

  Marked as helpful

  1

  ---

  

  Youssef Magdy•160

  @youssefmagdy21Posted about 2 years ago

  Thank you @correlucas for your feedback🙏🏻

  1. Great tip you pointed there on using a grid instead of a flexbox for the main container 👏🏻💯

  2. I already started using this plugin, it's great and very helpful thanks for suggesting it before👍🏻

  Thanks again for your feedback it's much appreciated 🙏🏻

  0
• 

  バレンタイン 😈•64,190

  @VCaramesPosted about 2 years ago

  Hey @youssefmagdy21, some suggestions to improve you code:

  • Your card layout is inconsistent. There are so many changes happening as change my screen size, even a few pixels.

  • You want add some reduce the amount of motion you card has, it can cause discomfort for some users. You can also add a Prefers-Reduced-Motion Property to your CSS code.

  • The Background Image Property is only to be used on decorative images. NOT images that add value and serve a purpose. For this challenge you want to use the Picture Element. By using this element not are able to use different size images, you can also save on bandwidth, meaning your content loads faster.

  Syntax:

    <picture>
      <source media="(min-width: )" srcset="">
      <img src="" alt="">
    </picture>
  

  Source:

  https://www.w3schools.com/html/html_images_picture.asp

  https://web.dev/learn/design/picture-element/

  • The Alt Tag description for the image needs to be improved upon. You want to describe what the image is; they need to be readable. Assume you’re describing the image to someone.

  • The old price isnt being announce properly to screenreaders. You want to wrap it in a Del Element and include a sr-only text explaining that this is the old price.

  • The "Button" is created with the wrong elements. It needs to be Button Element so that when the users clicks on it (with the help of JS), the product gets added to the cart.

  Happy Coding!

  Marked as helpful

  1

  ---

  

  Youssef Magdy•160

  @youssefmagdy21Posted about 2 years ago

  Thanks @correlucas, your suggestions are much appreciated🙏 i will make sure to keep in mind

  0

  

  Youssef Magdy•160

  @youssefmagdy21Posted about 2 years ago

  Hey again @vcarames

  I would like to thank you again for your feedback, your suggestions helped me improve my code. ⚡👍

  Please if you would like to, feel free to review the updated version. Your feedback would be very appreciated. 🙏

  0