Design comparison
Solution retrospective
Proud of:
I think that it looks correct! It has been a while since putting styling and front end work into practice so I'm happy that it turned out ok.
Next time:
Next time I would try organising the CSS a bit better, likely using a different CSS framework.
What challenges did you encounter, and how did you overcome them?Challenges:
It was only a very simple project so there wasn't much challenge apart from my memory failing with some things and needing to be reminded of how to properly use flex boxes and layouts.
What specific areas of your project would you like help with?I would like to know if my CSS could be written more elegantly, or in a particular way so as to scale better for larger projects.
I was also unsure if more should have been added for the desktop & mobile specs - both looked correct in chrome dev tools.
Community feedback
- @Islandstone89Posted 7 months ago
HTML:
-
Every webpage needs a
<main>
that wraps all of the content, except for<header>
andfooter>
. This is vital for accessibility, as it helps screen readers identify a page's "main" section. Wrap the card in a<main>
. -
Do not use words as "picture" or "image" in the alt text, as screen readers announce that automatically. The alt text must also say where it leads(frontendmentor website).
-
Headings should always be in order, so you never start with a
<h4>
. Since this is a component heading, it should be a<h2>
.
CSS:
-
Including a CSS Reset at the top is good practice.
-
Add around
1rem
ofpadding
on thebody
, so the card doesn't touch the edges on small screens. -
On the
body
, changeheight
tomin-height
- this way, the content will not get cut off if it grows beneath the viewport. -
Remove all widths and heights in
px
. -
Add a
max-width
of around20rem
on the card, to prevent it from getting too wide on larger screens. -
font-size
must never be in px. This is a big accessibility issue, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead. -
Since all of the text should be centered, you only need to set
text-align: center
on the body, and remove it elsewhere. The children will inherit the value. -
I would use
px
instead of%
forborder-radius
. -
On the image, add
display: block
andmax-width: 100%
- the max-width prevents it from overflowing its container. Remove the margin. -
To create the space between the image and the edge of the card, set
padding
on all 4 sides of the card:padding: 16px
. -
Whenever the left and right margins are identical, you can use the shorthand:
padding-inline: 32px
.
Marked as helpful2@i-prkrPosted 7 months ago@Islandstone89 Amazing feedback, just the kind of CSS guidance I was hoping for. Thanks for reasoning and links - appreciate your time!
1 -
- @MellTinsPosted 7 months ago
I think you did a great job!
I would just put a <footer> underneath with the <div> with your name and the name of the challenge in the <href> tags, just for consideration.
Marked as helpful1
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