Product Preview Card Component Solution

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted about 2 years ago

  Hi @MarwaanAbdalla 👋, good job completing this challenge, and welcome to the Frontend Mentor Community! 🎉

  I have some suggestions you might consider to improve your code:

  • Use the <main> tag to wrap all the main content in your solution instead of using <div class="container">.

  • Instead of using pixels in font size, use relative units of measure like rem or em. The font size in absolute length units (px) does not allow users with limited vision to change the text size in some browsers. Reference.

  • The <br> tag is not a semantic tag, so you should not use it. Also, if a screen-reader reads the text, it will break the flow of reading at the line break tag, so it should not be used to add vertical spacing. There are only a few cases where it is necessary (for example, in a poem or an address), and it is possible to avoid them by applying padding and margin styles via CSS. More information here.

  • You can use a <picture> tag when you need to change an image in different viewports. Using this tag will prevent the browser from loading both images, saving bandwidth and preventing you from utilizing a media query to modify the image.

  Example:

  <picture>
     <source media="(max-width: 600px)" srcset="./images/image-product-mobile.jpg">
     <img src="./images/image-product-desktop.jpg" alt="your_alt_text">
  </picture>
  

  I hope those tips will help you! 👍

  Good job, and happy coding! 😁

  Marked as helpful

  2

  ---

  

  Abdalla Moha•10

  @MarwaanAbdallaPosted about 2 years ago

  @MelvinAguilar Thanks for the help I would like to ask after changing from container to main tag the content of the page moved to left instead of center here is the updated code: https://github.com/shaalle/Product-preview-card-component

  1

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted about 2 years ago

  @MarwaanAbdalla You are using the class selector, but the <main> element does not have a class named main

  .main {
      . . .
  }
  

  Update the selector to

  main {
      . . .
  }
  

  Or add class "main" in <main> element <main class="main">, I recommend the first option

  Marked as helpful

  1

  

  Abdalla Moha•10

  @MarwaanAbdallaPosted about 2 years ago

  @MelvinAguilar Fixed it now thank you very much.

  1
• 

  バレンタイン 😈•64,170

  @VCaramesPosted about 2 years ago

  Hey there! 👋 Here are some suggestions to help improve your code:

  • To identify the main content of you site you will want to encase your entire component inside a Main 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.

  • This challenges requires the use of two images 🎑 for different breakpoints. The Picture Element will facilitate this.

  Here is an example of how it works: EXAMPLE

  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/

  • The old price 🏷 is not being announced properly to screen readers. You want to wrap it in a Del Element and include span element with an sr-only text explaining that this is the old price.

  If you have any questions or need further clarification, let me know.

  Happy Coding! 👻🎃

  Marked as helpful

  1
• 

  Mohamed Abdul Rahman•240

  @mdabdulrahmanPosted about 2 years ago

  Hi @MarwaanAbdalla,

  Welcome to Frontend mentor community, Your first solution is awesome and keep it up.

  Some advices 1.Wrap all the content in the <main> tag so you couldn't get an landmark error.

  2.Use the alt attributes for images

  Marked as helpful

  1