Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Product-Preview-card using Flexbox

Hazem Husseinβ€’ 120

@HazemHussein14

Desktop design screenshot for the Product preview card component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


This is my first challenge so I need your feedback, Thank you😍

Community feedback

@VCarames

Posted

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

@HazemHussein14

Posted

@vcarames Thank you for this valuable information about the picture element and of course for the interesting resources and techniquesπŸ“š 😍

0
Hassia Issahβ€’ 50,670

@Hassiai

Posted

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

@HazemHussein14

Posted

@Hassiai thanks for your feedback it was really helpful

0

@Abhirocks889

Posted

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

@HazemHussein14

Posted

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

1

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord