Frontend Mentor | Product preview card component

Community feedback

• 

  Miyamotto•50

  @MiyamottoPosted about 2 years ago

  Hey Lenz, i checked your code and can give you advice to use <picture> tag when you have more then 1 images and wrap them inside that tag. for example in your code remove your image tags and put this one there, under the article tag: <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>

  ..so when screen size is equal or less then 650px it will display mobile image, otherwise it will automatically resize to desktop version.

  Marked as helpful

  2

  ---

  

  Lenz Moraga•50

  @lenz-moragaPosted about 2 years ago

  @Miyamotto thanks for the comments, I updated the tag and it runs smoothly

  0
• 

  バレンタイン 😈•64,190

  @VCaramesPosted about 2 years ago

  Hey @lenz-moraga, some suggestions to improve you code:

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

  • This challenges requires you to use two images for different breakpoints. To properly achieve this, you want to use <picture> element.

  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 old price is not being announced properly to screen readers. You want to wrap it in a Del Element and include a sr-only text explaining that this is the old price.

  Happy Coding! 👻🎃

  Marked as helpful

  1

  ---

  

  Lenz Moraga•50

  @lenz-moragaPosted about 2 years ago

  @vcarames thanks for the comments, I Updated the alt text tag to be more readable and friendly to the user, and with the <picture /> tag it runs smoothly. Also, I changed the span tag for the previous price to a <del /> tag, didn't know it exists. I added some aria-label attributes to describe what it was without adding another element, but I found out that even if it works, it's not a good practice xD so I ended added an sr-only text as you mentioned.

  0