Product Card Review

Community feedback

• 

  Lucas 👾•104,340

  @correlucasPosted about 2 years ago

  👾Hello @LiamHallow, Congratulations on completing this challenge!

  Your solution its almost done and I’ve some tips to help you to improve it:

  1.The box-shadow is a bit too strong, this is due the opacity and blur. The secret to create a perfect and smooth shadow is to have low values for opacity and increase blur try this value instead: box-shadow: 12px 7px 20px 6px rgb(57 75 84 / 8%); If you’re not familiar to box-shadow you can use this site to create the shadow design and then just drop the code into the CSS: https://html-css-js.com/css/generator/box-shadow/

  2.Use a CSS reset to avoid all the problems you can have with the default CSS setup, removing all margins, and making the images easier to work, see the article below where you can copy and paste this CSS code cheatsheet: https://piccalil.li/blog/a-modern-css-reset/

  3.A better way to work this solution image, the product image is by using <picture> to wrap it on the html instead of using it as <img> or background-image (with the css). Using <picture> you wrap both images (desktop and mobile) and have more control over it, since you can set in the html when the images changes setting the screen size for each image.ote that for SEO / search engine reasons isn’t a better practice import this product image with CSS since this will make it harder to the image.

  Here’s the documentation and the guide to use this tag: https://www.w3schools.com/tags/tag_picture.asp

  See the example below:

  <picture>
    <source media="(max-width:650px)" srcset="./images/image-product-mobile.jpg">
    <img src="./images/image-product-desktop.jpg" alt="Gabrielle Parfum" style="width:auto;">
  </picture>
  

  ✌️ I hope this helps you and happy coding!

  Marked as helpful

  0
• 

  Oyero Habib•20

  @oyerohabibPosted about 2 years ago

  Nice work.

  However, can you clarify why you used, "position: absolute;" for your body?

  Also, in your media query, you don't need to your add the entire CSS attributes for the class, you only need to add the new changes.

  An instance here is, for the container class, the general usage was,

  .container {
      width: 350px;
      height: 600px;
      background: var(--White);
      display: flex;
      flex-direction: column;
      overflow: none;
      border-radius: 1rem;
      box-shadow: 0 5px 10px -3px #303030;
  }
  

  and in your media query, you had it as,

  @media screen and (min-width: 600px) {
      
      .container{
          width: 700px;
          height: 400px;
          background: var(--White);
          display: flex;
          flex-direction: row;
          overflow: none;
          border-radius: 1rem;
          box-shadow: 0 5px 10px -3px #303030;
      }
  }
  

  I think just having the differences in your media query is sufficient.

  @media screen and (min-width: 600px) {
      .container {
          width: 700px;
          height: 400px;
          flex-direction: row;
      }
  }
  

  This here would be sufficient.

  So you no longer need background, display, border-radius, or box-shadow since you are not making any changes to their values. And that would ultimately keep your CSS even shorter.

  Marked as helpful

  0

  ---

  

  Liam Hallow•40

  @LiamHallowPosted about 2 years ago

  @oyerohabib I was only doing some trial and error. I'll fix up on my lack of knowledge. thank you very much

  0
• 

  バレンタイン 😈•64,170

  @VCaramesPosted about 2 years ago

  Hey @LiamHallow, some suggestions to improve you code:

  • Your price shouldn't be wrapped in an h1 heading.

  • 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.

  • When using images that are different size for different breakpoints, its’ far more effective to use the <picture> element. By using this element not only 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>
  

  More Info:

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

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

  • For media queries, I definitely suggest using em for them. By using px your assuming that every users browser (mobile, tablet, laptop/desktop) is using a font size of 16px (this is the default size on browser). Em's will help with users whose default isn't 16px, which can sometimes cause the your content to overflow and negatively affect your layout.

  More Info:

  https://betterprogramming.pub/px-em-or-rem-examining-media-query-units-in-2021-e00cf37b91a9

  https://uxdesign.cc/why-designers-should-move-from-px-to-rem-and-how-to-do-that-in-figma-c0ea23e07a15

  • Your CSS Resets are extremely bare, you want to add more to it. Here are few CSS Resets that you can look at and use to create your own CSS Reset or just copy and paste one that already prebuilt.

  https://www.joshwcomeau.com/css/custom-css-reset/

  https://meyerweb.com/eric/tools/css/reset/

  http://html5doctor.com/html-5-reset-stylesheet/

  • While having interactive content (cards, links, icons, buttons, etc…) can definitely make content less static, if not done properly, it can actually have negative effect on your users experience. By simply just applying a “hover” effect to your content, you’re assuming that every device is compatible with “hover” effects. Unfortunately, most devices are not. To provide your users a better experience, you can use the @media (hover: hover) . Now users that that are devices that are not “hover” compatible will be able to enjoy your content.

  More info:

  https://css-tricks.com/solving-sticky-hover-states-with-media-hover-hover/

  https://youtu.be/uuluAyw9AI0

  Happy Coding!

  Marked as helpful

  0