Responsive | Product preview card component

Community feedback

• 

  Harsh Kumar•1,390

  @thisisharsh7Posted 8 months ago

  Hey, Nice work done. Your web page is well designed and is responsive. You uses flexbox in your solution which is the base for any responsive layout. Picture tag you use here is also a good approach for handling two different images at different screen size. Here I would like to suggest you one thing that you can use grid in the body tag to place the content i.e. card at center, this will saves you extra line of flex codes. You work is excellent.

  happy coding !!!!

  Marked as helpful

  1
• 

  T

  Grace•30,990

  @grace-snowPosted 8 months ago

  This is touching screen edges on all sides on my phone. Either the component needs margin or a wrapping element / body need padding.

  Other feedback

  • NEVER write font size in px. This is a serious accessibility failure.
  • don't style on IDs. Use classes. Styling is not what they are for and because every ID must be unique on a page means this component would not be reusable. I have a post all about IDs on the same site
  • the cart image is decorative so alt must be empty
  • you do not need a min width on the component anywhere.
  • border radius can be set on the whole component instead of child items. You will need to add overflow hidden to hide the overflowing child corners.
  • don't repeat the same properties in the media query if they don't change. That only adds bloat and damages performance.
  • the image width can be 100% if you want it to fill it's container.
  • the whole text part of the component should have padding on all sides. It's children should only need vertical margins (or gap on the parent would do the same thing). Make sure you understand the difference between padding and margin. I've written a post on that as well.

  Marked as helpful

  0

  ---

  

  P

  Mitali Shah•320

  @MitaliShahPosted 8 months ago

  Thank you so much, @grace-snow , for a thorough feedback

  Solution improvements:

  • Added margin on the component so that a card doesn't touch the sides. Replaced px to rems for font sizes.
  • Not styling on IDs anymore
  • The cart image is decorative, so I made the alt value an empty string.
  • Removed min-width from card
  • Removed border-radius from the child component, using overflow="hidden" to hide the overflowing child corner
  • Removed unused styling from media query.
  • Changed image max-width: 100%;
  • For the text part of the card, padding is added on all sides, and children now have only vertical margins.

  Let me know if you think I need to make more adjustments :)

  1
• 

  Gbenga Bayode•140

  @bammytech1Posted 8 months ago

  Well organised work..Happy coding

  1
• 

  P

  Mitali Shah•320

  @MitaliShahPosted 8 months ago

  Appreciate the feedback, Harsh! Thank you!

  0