@jakegodsall
Posted
Hi 👋
Great work on this project!
It really looks great at both mobile and desktop layouts 😁
The main issue I see is with larger viewports. I think it would make sense to talk about the width and height of the cards separately.
Width:
Right now you are using a vw
-based flex-basis
to determine the width of the columns in your flex container. This is fine for standard viewports, but with wider viewports the cards grow indefinitely, leading to lots of empty space within the card. I would recommend adding a max-width
with a fixed pixel value to each of the cards to stop this behaviour.
.card {
flex-basis: 20vw;
max-width: 300px;
}
Height:
Something similar is happening with the height becase of the vh
-based value for height
. Although the problem is the same, the solution is not. generally, it is best practice to not apply a fixed-value height
, because it can lead to content overflow, especially when our containers have dynamic content.
CSS is responsive by default and will determine the correct height for your component based on the size of the content, as well as padding
, border
etc, depending on the box-sizing
property you use.
I would recommend to leave the default height: auto
on your cards and use a padding-bottom
to add the desired whitespace below the content.
Hope this helps! 😁
Marked as helpful
@Kenn-eth
Posted
@jakegodsall Thank for your feedback.
@Kenn-eth
Posted
@jakegodsall I tried the solution but I got different result. I guess I followed the wrong approach from the start. I should have used grid instead of flexbox.
That said, I will keep your feedback in mind while working on the next challenge. Thank you 😃😃