@MelvinAguilar
Posted
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 addcursor: pointerto 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-boxproperty to make thewidthandheightproperties 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
@timilehin223
Posted
@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.
@MelvinAguilar
Posted
@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
