Product Preview Card Component

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted over 1 year ago

  Hello there 👋. Good job on completing the challenge !

  Your solution looks great, and you've done a fantastic job overall! However, I have other small suggestions to make it even better:

  HTML 🏷️:

  • The element <div id="cart"> should be a button and not a div. Since the element has a hover effect and is interactive, it's better to use a button element for accessibility. Don't forget to add cursor: pointer to indicate interactivity.

  • The <main> element should wrap around the entire component, not just part of it. This ensures that the main content of your page is correctly identified by assistive technologies and search engines.

  CSS 🎨:

  • You should use the box-sizing: border-box property to make the width and height properties include the padding and border of the element. This will make it easier to calculate the size of an element. You can read more about this here 📘.

  • It's better to apply styles using classes rather than IDs. Reserve IDs for JavaScript functionality and use classes for applying styles. This helps to avoid specificity issues.

  • Avoid applying fixed height to the component. Let the content determine the height so that it can adapt to different screen sizes and content lengths.

  • Instead of using vw units for font-size, consider using rem or em units. This will help to maintain better consistency across different devices and avoid potential issues on some screens.

  • When creating two columns with Flexbox, use width: 50% on both columns to make them equal in size.

  I hope you find it useful! 😄

  Happy coding!

  Marked as helpful

  8

  ---

  

  Mustapha Al-Amin Oluwatimmilehin•90

  @timilehin223Posted over 1 year ago

  @MelvinAguilar Regarding the height, I wanted to do use a unit that will make the look consistent across various viewport sizes. If I were to make the height adapt to the content, I am not sure how the responsiveness will behave. Same goes to the font-size units.

  0

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted over 1 year ago

  @timilehin223 Hello again! I understand your concern about maintaining consistency across different viewport sizes. If you want to set a height for the container, you can use min-height instead of height and use units like px, rem, or em, never height with vh in this type of component.

  Currently, using height: 60vh in the .container is causing two issues:

  • screenshot-1: In the first scenario, on a mobile device in landscape mode, the height limitation at 60vh causes the component to never grow beyond 60% of the screen height. As a result, the content may overflow, and elements like the price and button might appear outside the component.

  • screenshot-2: In the second scenario, on larger screens like a 4K display, using 60vh for height will make the component excessively tall and look unusual on such high-resolution screens.

  Keep up the great work, and happy coding! 🚀

  Marked as helpful

  1
• 

  Jonathan•20

  @jchu51Posted over 1 year ago

  LGTM.

  • It's great to see you're using picture tag, to handle image

  • Try to avoid using IDs / elements selector for CSS styling. Class selectors are generally preferable as they're reusable and don't have the high specificity that IDs have, reducing the risk of style conflicts.

  • Use rem for consistent sizing across your website. The sizes are based on the base font-size, and they don't change with the parent element's font-size.

  2

  ---

  

  Mustapha Al-Amin Oluwatimmilehin•90

  @timilehin223Posted over 1 year ago

  @jchu51 To make the font-size responsive, I wanted to use a unit that will change with the size of the viewport.

  0