Design comparison
Solution retrospective
I'm happy that I finally did a challenge
I need to plan better and learn more about the fundamentals
What challenges did you encounter, and how did you overcome them?I struggled with centering the divs and the image and I did not know how to use margins and paddings
I searched a lot of questions on google and learnt from them
Once I was completey stuck and didn't understand why the image wasn't centered properly, I watched a video of the solution and this taught me about css variables and the max-width property, it also taught me why the align-item property wasn't working
What specific areas of your project would you like help with?I did not really pay attention to the accessibility, and I would like to know how to make it accessible
I want to know how to make my code more efficient or readable, or if there's some shortcuts and what are the best practices, should I use other units instead of px
And how do I layout the html
Community feedback
- @Islandstone89Posted 4 months ago
Hello, good job!
Looking through your code, these are my suggestions to improve your solution even further. I hope you find this feedback clear and helpful :)
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>
. -
The alt text must also say where it leads(the frontendmentor website). A good alt text would be "QR code leading to the Frontend Mentor website."
-
Headings should always be in order, so you never start with a
<h3>
. I would make it a<h2>
- a page should only have one<h1>
, reserved for the main heading. As this is a card heading, it would likely not be the main heading on a page with several components. -
.attribution
should be a<footer>
, and you should use<p>
for the text inside.
CSS:
-
Including a CSS Reset at the top is good practice.
-
I like to add
1rem
ofpadding
on thebody
, to ensure the card doesn't touch the edges on small screens. -
Remove the
height
inpx
on thebody
. Never set fixed heights on elements containing text! If the content in the body grew to be taller than580px
, it would cause overflow. The content should decide how tall the body is. The body is by default only as tall as its content. Since we want to center the card vertically, we do need to create vertical space to center it in. The way to do that is to setmin-height: 100svh
on thebody
- this means the body will take up at least the full viewport height, but since it's a min-height, it has also room to grow if needed. -
Remove the
width
inpx
on the card. It is not recommended to set fixed sizes inpx
, unless sometimes for small things like icons. Instead, we want our components to adapt to all the different kinds of screens it can be viewed on. -
We do want to limit the width on the card to an extent, so it doesn't stretch too wide on large screens. We do this by adding a max-width, which should be in rem. Add
max-width: 20rem
on the card. -
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. -
Well done for having
max-width: 100%
on the image! Images, by default, take up the width of their intrinsic size, so if you have a card that is20rem
wide (equals320px
) and the image is500px
wide, it would overflow out of the card. Settingmax-width: 100%
makes the image shrink according to its container. Hence, it's best practice to set this on all images in every project you do. It's also common to adddisplay: block
on images, otherwise you might end up with a bit of unwanted space under images, as they are an inline element. -
Finally, I would probably slightly increase the
padding
on the card, to around16px
.
Keep up the good work!
Marked as helpful1@pudding-sharkPosted 3 months ago@Islandstone89
Thank you for your response
It was very digestible (I am bit slow though so I might have missed something)
I made some adjustments based on your feedback. I also thank you for providing useful blogs/posts
I added you under "acknowledgements" in the README.md thing because you helped a lot. Hope you don't mind
1 -
- @RabelahmedPosted 4 months ago
Congratulations on completing your project. min-height:100vh; on body element to center your container horizontally and vertically also instead of using h3 use h1 skipping level is not semantic html if you need smaller font just change font size hope this helps
2@pudding-sharkPosted 3 months ago@Rabelahmed
Thank you for your response
I added you under "acknowledgments" in the README.md thing.
Hope you don't mind
0 - @PropowershellPosted 3 months ago
Hello Pudding-Shark, You're QR code component is looking good so far but there are just two things I wanted to point out.
-
The "Improve" in the heading element seems to be in serif font, maybe you should place the import link (<link href='https://fonts.googleapis.com/css?family=Outfit' rel='stylesheet'>) in the head tag in the HTML file.
-
Use Slate 900: hsl(218, 44%, 22%) for the heading element's color(it's recommended you use a h2 element since it's not the first heading tag in the page.
Other than that it's great.
0 -
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