Product Preview Card Component with React & Tailwindcss

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted 12 months ago

  Hello there 👋. Good job on completing the challenge !

  I have other suggestions about your code that might interest you.

  • Utilize min-h-screen (or min-height: 100vh) instead of h-screen to prevent your component from getting shortened on mobile devices.

  • The image has a margin that is too large, specifically md:ml-[252px]. Consider removing it for better alignment.

  • The issue I notice is that you are creating two columns with grid-cols-2 but using the same element to center them. As mentioned by Vishnu_31, consider creating another div to exclusively contain these two columns for better structure. like this:

    <div class="bg-cream min-h-screen w-screen flex flex-col items-center justify-center">
        <div class="grid grid-cols-1 md:grid-cols-2">
            <article ...></article>
            <div ...></div>
        </div>
    </div>
    

  • You can use the <picture> tag when you have different versions of the same image 🖼. Using the <picture> tag will help you to load the correct image for the user's device saving bandwidth and improving performance. You can read more about this here 📘.

  I hope you find it useful! 😄 Above all, the solution you submitted is great!

  Happy coding!

  Marked as helpful

  0

  ---

  

  Bongani Masango•100

  @bkarabo754Posted 12 months ago

  Thank you so much @MelvinAguilar, this is really helpful. I will look into it and make the necessary changes. Thank you once again, sir. :)

  1
• 

  Tesla_Ambassador•3,000

  @tesla-ambassadorPosted 12 months ago

  Hey Bongani Masango, Congrats on completing your challenge! I had a look at your code and found that you set flex-col without setting flex first in your tailwind css. This would have enabled you to use flex instead of grid which would be great for this solution and then you'd change to flex-row at the desired screen width. This would prevent your divs from separating. Otherwise, I think this is really great!

  Marked as helpful

  0

  ---

  

  Bongani Masango•100

  @bkarabo754Posted 12 months ago

  Hi @tesla-ambassador, I appreciate your feedback, I will look at it. Thank you for pointing that out sir. :)

  1
• 

  Vishnu_31•230

  @vishnu-31Posted 12 months ago

  Hello there. Congrats on completing the chalenge.

  I have some suggestions to improve.

  • you should create another div and have these 2 'div's inside it and give p-0 (no padding).
  • and also m-0 *(no margin)*and remove other margin elements on these 2 divs -this will remove the space between the divs and show them always together at all the screen size.

  Marked as helpful

  0

  ---

  

  Bongani Masango•100

  @bkarabo754Posted 12 months ago

  @vishnu-31 Thank you for the feedback, sir. I will work on it :)

  0