Product-Preview-card using Flexbox

Community feedback

• 

  バレンタイン 😈•64,170

  @VCaramesPosted almost 2 years ago

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

  Congrats on completing your first challenge!🎊🎆

  • Since the images in this component add value and serve a purpose (displaying the product), it is best to use the Picture element and not the background-image property, as it will to use different images during different breakpoints.

  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

  • The name of the perfume , “Gabrielle Essence Eau De Parfum”, is the most important content in your card so it should be wrapped in a heading Element.

  • Currently, the old price (169.99) 🏷 is not being properly announced to screen readers. To fix this, you are going to wrap the the price in a del element and inside it you will add a span element with an sr-only class that will state something like “The previous price was…” and use CSS to make it only visible to screen readers.

  More Info:📚

  Del Element

  • Implement a Mobile First approach 📱 > 🖥

  With mobile devices being the predominant way that people view websites/content. It is more crucial than ever to ensure that your website/content looks presentable on all mobile devices. To achieve this, you start building your website/content for smaller screen first and then adjust your content for larger screens.

  More Info:📚

  Mobile First

  • Your CSS Reset is extremely bare and being underutilized. To fully maximize your CSS reset, you want to add more to it.

  Here are few CSS Resets that you can look at and use to create your own or just copy and paste one that is already prebuilt.

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

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

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

  If you have any questions or need further clarification, feel free to reach out to me.

  Happy Coding!🎄🎁

  Marked as helpful

  1

  ---

  

  Hazem Hussein•120

  @HazemHussein14Posted almost 2 years ago

  @vcarames Thank you for this valuable information about the picture element and of course for the interesting resources and techniques📚 😍

  0
• 

  Hassia Issah•50,650

  @HassiaiPosted almost 2 years ago

  Replace <div class="container"> with the main tag, <p class="product-name"> with <h1> and <div class="attribution"> with the footer tag to fix the accessibility issues. for more on semantic html visit https://web.dev/learn/html/semantic-html/

  To center .container on the page, add min-height:100vh; display: flex; align-items: center: justify-content: center; or min-height:100vh; display: grid place-items: center to the body. and replace margin: 200px auto; with margin: 0 auto;

  Use rem or em as unit for the padding, margin, width and preferably rem for the font-size for more on this watch this https://youtu.be/N5wpD9Ov_To

  Hope am helpful.
  HAPPY CODING

  Marked as helpful

  1

  ---

  

  Hazem Hussein•120

  @HazemHussein14Posted almost 2 years ago

  @Hassiai thanks for your feedback it was really helpful

  0
• 

  Abhishek Buragohain•120

  @Abhirocks889Posted almost 2 years ago

  Hello Hazem, first of all congratulations on submitting your first challenge. I am sure this is the beginning of many more to come.

  Regarding the solution, here are some of my pointers:

  1. The design looks great. I particularly like the addition of the attribution.
  2. The solution is responsive too. So, that is wonderful. However, for widths <= 600px, you could consider adding some padding to the body so that the card always has some space around it. Something like:

  @media (max-width: 600px) {
   body {
    padding: 20px;
   }
  }
  

  The other main feedback is regarding semantic HTML. Generally, it is considered a better practice to use semantic elements like (section, article, header, etc.) instead of plain div and span. This is to improve accessibility. You can read more about it here: Importance of Semantic HTML

  Again, congratulations on your first solution and keep learning and growing.

  Regards, Abhishek

  Marked as helpful

  1

  ---

  

  Hazem Hussein•120

  @HazemHussein14Posted almost 2 years ago

  @Abhirocks889 Thank you I will modify the code it was a nice idea

  1