Responsive Product Review Card

Community feedback

• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted 24 days ago

  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•150

  @Ahmed-NafrawyPosted 24 days ago

  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•150

  @Ahmed-NafrawyPosted 24 days ago

  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

  Øystein Håberg•10,710

  @Islandstone89Posted 24 days ago

  @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•150

  @Ahmed-NafrawyPosted 24 days ago

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

  1
• 

  Jaypo16•30

  @Jaypo16Posted 24 days ago

  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