Some flexbox and some JS.

Community feedback

• 

  Hassia Issah•50,650

  @HassiaiPosted almost 2 years ago

  Replace<div class="container">with the main tag and <div class="attribution"> with the footer tag to fix the accessibility issues. click here for more on web-accessibility and semantic html

  To center .container on the page using flexbox, replace the height in the body with min-height:100vh.

  Use relative units like rem or em as unit for the padding, margin, width values and preferably rem for the font-size values, instead of using px which is an absolute unit. For more on CSS units Click here

  Hope am helpful.

  Well done for completing this challenge. HAPPY CODING

  Marked as helpful

  1
• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hello there 👋. Good job on completing the challenge !

  I have some suggestions about your code that might interest you.

  HTML 📄:

  • It is respectable to use JavaScript, you have used it to add a class to discounts if there is an original price, you can do it directly in the HTML and save all the JavaScript code, you should use JavaScript as a last resort, use it to give more behavior to the page, but it is good to practice it to improve your skills.

  • Use the <main> tag to wrap all the main content of the page instead of the <div> tag. With this semantic element you can improve the accessibility of your page.

  • Use the <footer> tag to wrap the footer of the page instead of the <div class="attribution">. The <footer> element contains information about the author of the page, the copyright, and other legal information.

  • You could use the <del> tag to indicate the price that was before the discount. Additionally, you can use a sr-only class to describe the discount. This will help screen reader users to understand that the price was discounted.

    Example: <del><span class="sr-only">Old price: </span>$169.99</del>

  • You can use the <picture> tag when you have different versions of the same image 🖼. Using the <picture> tag will help you to load the correct image for the user's device saving bandwidth and improving performance. You can read more about this here 📘.

    Example:

    <picture>
        <source media="(max-width: 460px)" srcset="./images/image-product-mobile.jpg">
        <img src="./images/image-product-desktop.jpg" alt="{your alt text goes here}">
    </picture>
    

  CSS 🎨:

  • Use min-height: 100vh instead of height: 100vh. The height property will not work if the content of the page grows beyond the height of the viewport.

  I hope you find it useful! 😄 Above all, the solution you submitted is great!

  Happy coding!

  Marked as helpful

  1

  ---

  

  Pierre Allard•135

  @RondellAllardPosted almost 2 years ago

  @MelvinAguilar

  I don't see how i could use HTML in the same way I used the JS. I wanted it to be conditional. If there is a sale price or new price how it is presented in CSS will change.

  Thank a million for the lengthy review. Haven't done this in a while.

  1

  

  Pierre Allard•135

  @RondellAllardPosted almost 2 years ago

  @RondellAllard thanks for the sr-only class as well.

  1

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  @RondellAllard You're welcome.

  0
• 

  バレンタイン 😈•64,170

  @VCaramesPosted almost 2 years ago

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

  • Do not forget ⚠️ to check your FEM report (It provides value information), to see what is incorrect and update your code with it. This should be done immediately after submitting your challenge.

  • Every site should ALWAY have ✅ a main element not only for semantic purposes but also to help assistive technology find the main content of your content. For this challenge, it will serves as the component’s container ⚠️.

  More Info: 📚

  MDN Main Element

  • The only heading ⚠️ in this component, is the name of the perfume; “Gabrielle Essence Eau De Parfum” . The rest of the text should be wrapped in a paragraph element.

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

  Happy Coding! 🎆🎊🪅

  Marked as helpful

  0

  ---

  

  Pierre Allard•135

  @RondellAllardPosted almost 2 years ago

  @vcarames wouldn't perfume also be inside of the heading tag?

  0

  

  バレンタイン 😈•64,170

  @VCaramesPosted almost 2 years ago

  @RondellAllard

  This component is part of a larger site so having "perfume" makes no sense since headings are meant to provide structure to you content.

  1

  

  Pierre Allard•135

  @RondellAllardPosted almost 2 years ago

  @vcarames so that would be in a p tag as well?

  0

  

  バレンタイン 😈•64,170

  @VCaramesPosted almost 2 years ago

  @RondellAllard

  Correct ✅

  0

  

  Pierre Allard•135

  @RondellAllardPosted almost 2 years ago

  @vcarames is there something wrong with the "generate report"?

  I am not finding the validation error they speak about.

  This svg:no

  Edit. I can't the the HTML to show....

  0

  

  バレンタイン 😈•64,170

  @VCaramesPosted almost 2 years ago

  @RondellAllard

  Odd. You can try generating a new report and see if it goes away.

  1