Antonius Steven• 100
@ASteven21
Submitted
I would love to hear feedbacks on how to make it even better. Thanks!!
Thiago Souza
@thiagorasouza
All comments
- Thiago Souza• 80
@thiagorasouza
Posted
Hi Antonius!
Great solution! 🥳
I loved how simple your layout is. You managed to make it all work with few Flexbox uses. Nice use of min-* max-*, it is all flowing like it should. I will definitely take an approach similar to yours in my next challenge.
I also noticed you used background-repeat: space, which I did not know it existed until reading your solution.
That being said, there is only one thing I could add:
- You can remove position: relative and bottom: XXXrem on .left-card and .middle-card if you take advantage of flexbox's align-item property in conjunction to margin-bottom. Only margin-bottom won't do the trick, because when you create your flex container on .cards-section using display: flex, CSS defaults the value of justify-content to "flex-start" (items align to start of the main axis - left of the container) and align-items to "stretch" (items stretch in the cross - vertical - direction). So, you need to set .cards-section's align-items property to "flex-end" and them add, for example, margin-bottom: 1em to .middle-card and margin-bottom: 2em to .left-card.
If my feedback helped you in any way, please consider upvoting it!
Also, it would be great to receive your feedback on my solution too 😄
1 - Izabela Górska• 100
@IzaGorska
Submitted
Thiago Souza• 80@thiagorasouza
Posted
Hey Izabela Górska!
Good work! Intuitive HTML markup and good code structure.
I have a few comments to make:
- Using grid-template-areas on div.comments > div would make that part of your code more readable;
- When applying align-self to .name elements, it's better to use "end" instead of "flex-end", since the container is a CSS Grid in this case, not a Flexbox;
- On the desktop version of div.comments, the rule grid-template-areas: ". . ."; (dot dot dot) is not necessary. Since you are not creating any references/template and grid-template-rows and grid-template-columns are already set, there is really no need.
- Your .comments are overflowing a little bit on smaller window widths, probably because you have set a fixed height for them. Instead you could set height: auto; and align-self to "start".
- The background images are not reaching the bottom end of the window. That's tricky to solve, but doable. The body element is currently only as tall as the content and you need it to be as tall as the window viewport. To solve that, you can set body's min-height to 100vh. Observe now that the page is overflowing, vertical scrollbars appear because the .container's margins are collapsing. In other words, the .container margins end up outside it's parent element (body). So body is actually 100vh + .container's margin-top + .container's margin-bottom. In your case, the fastest way to solve that is to change your margins declarations to padding on .container. Look for "mdn margin collapsing" to read more on that.
If my feedback helped you in any way, please consider upvoting it!
Also, it would be great if you could give me your feedback on my solution 😀
1 - Taha• 310
@thaykrgl
Submitted
I would be very happy if you could help me with the parts I need to improve. Thank you for your feedback.
Thiago Souza• 80@thiagorasouza
Posted
Hey Taha!
Great markup and CSS code structure. Everything is simple and easy to understand.
That being said, I have a few comments:
- Setting a padding-top of 10vh on your .container element may be a waste of space on the mobile layout. Mobile users don't really have much screen space. Instead of setting a padding-top on the .container element, you could try to page center it (both on the desktop and mobile layout). You can look up the web for "centering a div in body element". I personally prefer to turn body into a flex container (display: flex), give it a height (min-height: 100vh), a vertical flow (flex-direction: column), align it's items to the center of the main axis (justify-content: center) and the same for the cross axis (align-items: center). For that to work, you need to remove margin: auto from your .container element;
- Try horizontally resizing your window with Chrome/Firefox Dev Tools (F12). Your media screen breakpoint is pretty small. There's a point between 900px and 400px where your text begins to squeeze. You can either set a higher value for your breakpoint or change the way you set your .container width;
- And I think you missed the Sign Up button hover state, but that's easy to implement.
That's all! Good work! I would appreciate if you could give me your feedback on my solution too. If my feedback helped you in any way, feel free to upvote it :)
1
