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 component - simple grid

Ritesh Kumarā€¢ 90

@riteshkumarldh

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


please provide feedback about my code

Community feedback

Liam Tanfieldā€¢ 260

@TanDevv

Posted

Hi, riteshkumarldh! Great work on this one! Here are a few suggestions regarding your code. :)

šŸ“HTML:

  • In regards to your <img src="./images/icon-cart.svg" alt="cart">, I would suggest leaving any icons/decorative images (eg: pattern backgrounds) that hold no meaningful content with an empty alt attribute, so screen-readers will ignore it and not confuse the users depending on them.

(For more information regarding <alt>, you can read this article also by MDN)

šŸŽØ CSS:

  • Try to avoid using px units for everything. Pixel units do not scale well once you take into account users who may increase the font size via their browser for accessibility, if the website explicitly sets font-sizes in pixels, a heading set at 30px will always be 30px. Which may cause issues for users with visual impairment. rem or em units both respectfully scale when needed and would be much preferred when it comes to font sizing, padding/margins and media queries.

If you want something to be a constant size that does not need to be responsive to changes in other elements or little things such as borders, border radius or box shadows then pixel units are the way to go.

(For more information on using units and which are best for what, Kevin Powell does a great video explaining a few of them)

.btn {
    display: inline-block;
    width: 100%;
    background-color: var(--dark-cyan);
    border: none;
    outline: none;
    height: 60px;
    border-radius: 10px;
    font-size: 18px;
    font-weight: 700;
    color: var(--white);
    display: flex;
    align-items: center;
    justify-content: center;
    gap: 10px;
}
  • Regarding to your code above, you have two duplicate display rules, I would suggest removing one of them as the one below the other will just overwrite it, making it redundant.

I hope you find this information helpful. Above all, your solution is great, well done! šŸ˜„

Marked as helpful

0

Ritesh Kumarā€¢ 90

@riteshkumarldh

Posted

@YorkieLT Hi, very helpful feedback will keep in mind all the mentioned points thanks.

0

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