Design comparison
Solution retrospective
All feedback is welcome
Community feedback
- @antaryaPosted about 2 years ago
Hi ,
Great job, it looks good.
I have a couple of suggestions:
CSS
[1. All elements inside
.whole
havemargin-left: 40px
. To achieve the same but to have only one style, you can define padding on the.whole
element. It will be easier in the future to change it instead of specifying it for each child element./* remove all margin-left: 40px on all child elements and add one padding to parent */ .whole { padding: 17px 40px; }
[2. The content inside the
Add to Cart
button is not aligned vertically; it looks like the fixed height is redundant..whole .fa { /* width: 50px; */ /* height: 100px; */ margin-right: 20px; }
[3. You have fixed
width
andheight
which works in this case, but you cannot depend on text length; in real applications, you never know the size of the length, so you need to make your layout adapt to whatever length it will have. You can check it by typing longer text for an item description. Allow elements to grow without fixed width/height, and when you want to restrict, usemax-width
/max-height
ormin-width
/min-height
;[4. On smaller screens, e.g.
320px
, the page doesn't look like in mock. You might need to use media queries, so it looks good on smaller screens.- https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries,
- https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Responsive/Mobile_first
And for arranging items, try using flexbox or grid as it is easier to use and has more features.
- https://css-tricks.com/snippets/css/a-guide-to-flexbox/
- https://www.frontendmentor.io/resources search for flexbox and grid for useful learning material.
Using a mobile-first approach and media queries, you will change styles to:
.container { /* A B */ display: flex; flex-direction: column; /* you can set radius on parent element instead of each child element */ border-radius: 10px; overflow: hidden; } @media (min-width: 900px) { .container { /* A B */ /* display: flex; */ flex-direction: row; } }
[5. You can set
display: block
on the image to remove white space under the image. For details, check:HTML
[6. Wrap your card container with the
main
tag instead of using themain
tag as your card parent tag; this would be semantically correct.- https://learn-the-web.algonquindesign.ca/topics/html-semantics-cheat-sheet/
- https://www.w3schools.com/html/html5_semantic_elements.asp
Let me know if you have questions. I hope this will be helpful.
Keep up the good work . Cheers!
Marked as helpful0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord