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

Responsive Product Review Card

@Ahmed-Nafrawy

Desktop design screenshot for the Product preview card component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Community feedback

P

@Islandstone89

Posted

Great job, Ahmed!

A few suggestions :)

HTML:

  • <main> holds all of the main content on a page. As a card would likely not be the only component on a page, I would wrap the card content in a <div class="card"> inside of <main>.

  • I would change the heading to a <h2> - a page should only have one <h1>, reserved for the main heading. As this is a card heading, it would likely not be the main heading on a page with several components.

  • On the prices, I would use <p> instead of <span>. Instead of using a title attribute, I would add <span class="visually-hidden">Old price:</span>, then style it in CSS:

.visually-hidden {
    clip-path: inset(50%);
    position: absolute;
    overflow: hidden;
    white-space: nowrap;
    width: 1px;
    height: 1px;
}

This article explains what each of the properties does.

  • As the cart icon is purely decorative, the alt text should be empty:alt="".

CSS:

  • Including a CSS Reset at the top is good practice.

  • On the body, change height to min-height: 100svh - this way, the content will not get cut off if it grows beneath the viewport.

  • Here is why you should not change the font-size to 62.5%.

  • letter-spacing must not be in px. You can use em, where 1em equals the element's font size.

Marked as helpful

1

@Ahmed-Nafrawy

Posted

Helpful as usual 💓 Thank you so much for the effort!

And for the <p> I did use it at first, but when I submitted the solution, front-end mentor report said that I shouldn't use a <p> inside a button element, instead I should use a span, and I did that to avoid that error

So, which is right? @Islandstone89

1

@Ahmed-Nafrawy

Posted

And yeah, the same goes for the <h1> , cuz I know for sure that this heading won't be the first one in my page but I also used it here to avoid that message at frontend mentor report

And also for the <main>

But thanks a lot @Islandstone89

1
P

@Islandstone89

Posted

@Ahmed-Nafrawy Happy to help!

You can use a <span> inside the button, I was referring to the prices - I don't think it matters too much, though :) I also think you don't need to wrap button text in an element - I usually have the text directly inside the button.

Marked as helpful

1

@Ahmed-Nafrawy

Posted

Okay I will do that next time , I appreciate your work 💓@Islandstone89

1
Jaypo16 30

@Jaypo16

Posted

Very nice webpage looks exactly like the example and nice clean code. I like how it smoothly transitions from Mobile to desktop keeping the same size of picture without any stretching or distortion of the picture good job.

Marked as helpful

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