Design comparison
Community feedback
- @Islandstone89Posted about 1 year ago
Hi, well done submitting your solution. Here are some suggestions to make this even better.
HTML:
-
Every page should have a
<main>
for the main content. I would change your.overall-container
to<main class="container">
. -
Don't use
<br>
to style the text. HTML is all about content, let the CSS do the presentation. -
.improve
should be a heading. -
For a challenge as simple as this one, you don't need to wrap the image in a div.
CSS:
- First, you need some basic CSS "resets" at the top of the stylesheet:
*, *::before, *::after { box-sizing: border-box; } img { display: block; max-width: 100%; }
-
On the
<body>
, changeheight
tomin-height
. Also, addalign-items: center
to center the card vertically. -
Hence, you can remove the margin on the card - the card will be centered horizontally and vertically using Flexbox. Do also remove the fixed height and width, we generally don't want to set fixed dimensions. But add a
max-width
in rems on the card. -
Remove the margin from the image. To get the space around the image, use padding on the card itself.
-
You are using the wrong font. See the style guide for the link to the correct Google Font. Also, the font-family should be set on the
body
. -
Since all text on the card should be center-aligned, it's better to set
text-align: center
only once, on thebody
. The text elements are children (or technically grandchildren) of the body, and so they will inherit its values.
Hope this is clear and helpful :)
Marked as helpful0@Mastermind390Posted about 1 year ago@Islandstone89 Thanks for your suggestions and I really appreciate.
I will look into all what you pointed out and make necessary changes.
Thanks once again.
1 -
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