Responsive product preview card

Community feedback

• 

  anisbeny•120

  @anisbeny

  Posted about 1 year ago

  Thank you for your feedback, I really appreciate your recommendations.

  00
• 

  David Dutra•230

  @dvdgd

  Posted about 1 year ago

  Hello! You've done a great job—it's nearly perfect.

  I just have a few points to cover to help you improve it even further:

  • While viewing on a mobile screen, I noticed that the image source doesn't change. You can fix this by replacing the figure with a picture and adding different sources with breakpoints. Here's an example:

    <picture>
      <source media="(min-width: 768px)" srcset="./images/image-product-desktop.jpg">
      <img src="./images/image-product-mobile.jpg" alt="Perfume bottle">
    </picture>
    

    When using picture, ensure to set the display property to contents: display: contents.

    • By default, the <picture> element behaves like a block-level element, meaning it takes up the full width of its parent container. However, the <img> element inside it behaves like an inline-level element. This means that any padding, margins, or borders applied to the <picture> element will affect its layout, potentially causing unintended spacing or alignment issues.

    • Setting the display property of the <picture> element to contents alters its behavior. The contents value causes the <picture> element to behave as if it were not there in terms of layout, essentially making its children (in this case, the <source> and <img> elements) direct children of the <picture> element's parent. This ensures that any styles applied to the <picture> element itself do not interfere with the layout of its children.

  • I noticed that the card grows on mobile but doesn't shrink less than 350px due to your clamp(350px, 75%, 750px). Here's how to address this:

    • Remove the clamp for the min-width of the article.
    • Add a max-width of 350px for the card on devices with screens smaller than or equal to 768px. This way, you'll only need to consider two variations: desktop and mobile. This ensures the card doesn't grow excessively but can still shrink for very small screens.

  • For your price, you could add flex-wrap: wrap. This will prevent the price from overflowing on small screens.

  TIP: For desktop, the max-width of the card could be 600px, and for mobile devices 350px. This could simplify centering the card using flex-box.

  PLUS: There are several ways to center an element on the page. But instead of using position and transform, you could utilize flex-box, where it remains a cleaner way and helps us deal with responsiveness, and I saw you already know how it works. Here's an example::

  body {
    display: flex;
    justify-content: center;
    align-items: center;
    min-height: 100svh; /* Ensure your page occupies all available height */
  }
  

  I hope this feedback helps you improve your solution. Feel free to ask anything, and if you'd like, I can create a PR on GitHub to assist you further!

  I appreciate how you've made your HTML semantic and opted for CSS selectors instead of creating classes—awesome!

  Keep up the good work and happy coding!

  00
• 

  homciAbdallah•210

  @homciAbdallah

  Posted about 1 year ago

  Does the solution include semantic HTML? Yes in deed Is it accessible, and what improvements can be made? Can and improvements in size Does the design look good on a range of screen sizes? Yes Is the code well structured, readable, and reusable? Yes Is the solution significantly different from the design? a little

  00