Skip to content
Submitted about 2 years ago

Responsive page design using CSS-Flexbox

Mohammad Raza•120
@Raza7522
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Product preview card component
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Solution retrospective

It's my first project on Frontend-Mentor. Please let me know if my solution is up to the mark or something missing related to best practices.

Community feedback

  • Melvin Aguilar 🧑🏻‍💻•61,140
    @MelvinAguilarPosted about 2 years ago

    Hi @Raza7522 👋, good job completing this challenge, and welcome to the Frontend Mentor Community! 🎉

    Here are some suggestions you might consider:

    • In this challenge, you should not use the background property to set the image because this image has semantic meaning. Use the CSS background property if the image is not part of the content.

    You can use a <picture> tag when you need to change an image in different viewports. Using this tag will prevent the browser from loading both images, saving bandwidth and preventing you from utilizing a media query to modify the image.

    Example:

    <picture>
   <source media="(max-width: 376px)" srcset="./images/image-product-mobile.jpg">
   <img src="./images/image-product-desktop.jpg" alt="your_alt_text">
</picture>

    With semantic tags:

    <body>
   <main class="container">
      . . .
   </main>
   <footer>
      . . .
   </footer>
<body>

    • The <h1> element is the main heading on a web page. There should only be one <h1> tag per page, and always avoid skipping heading levels. The HTML Section Heading elements (Reference). Your "active-price" element should not be a heading element.

    • You could use the <del> tag to display the old price:

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

    Note that I added the <span> with the sr-only class to the del element, this will provide more information about what your old price is about.

    The sr-only class is a class that you can add to hide content visually but is only visible to screen-readers.

    More information here.

    I hope those tips will help you.

    Good job, and happy coding!

    Marked as helpful
    1
    Mohammad Raza•120
    @Raza7522Posted about 2 years ago

    @MelvinAguilar Thanks for giving such a valuable feedback and highlighting so many points. I will Definitely try to apply those points.

    1
  • 👾 Ekaterine Mitagvaria 👾•7,600
    @catherineisonlinePosted about 2 years ago

    On Chrome, at around 469px screen size, the content container doesn't fit the content anymore and creates a scroll inside the content container + the button is half visible which doesn't look really nice. I would probably change the height so it fits and you don't have to scroll down. By content container I mean the div with class "'content-section"

    Marked as helpful
    1
    Mohammad Raza•120
    @Raza7522Posted about 2 years ago

    @catherineisonline Thanks for your valuable feedback. I will definitely try to fix that issue.

    0

Please log in to post a comment

Log in with GitHub

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