Design comparison
Community feedback
- @Islandstone89Posted 3 months ago
Hi, good job.
Here are some things that need to be improved. I hope you take them on board, and apply the necessary changes to your code :)
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 should be written with normal capitalization, and it 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 change it to 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.
-
Remember to specify a fallback font:
font-family: 'Outfit',sans-serif;
-
I like to add
1rem
ofpadding
on thebody
, to ensure 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. -
Similarly, you can set the
font-family
on thebody
, and remove it elsewhere. -
On the image, add
display: block
and changewidth
tomax-width: 100%
- the max-width prevents it from overflowing its container. Remove thepadding
. -
To create the space between the image and the edge of the card, set
padding
on all 4 sides of the card:padding: 16px;
. -
I would use
px
instead of%
forborder-radius
. -
Instead of having descendant selectors like
.qr-content p
, it is recommended to give elements a class, for example<p class="paragraph">
.
Marked as helpful1@Liyyy9Posted 3 months agoHi @Islandstone89,
Thank you so much for your thoughtful feedback! I genuinely appreciate the time you took to review my work and offer such detailed suggestions. Your insights are incredibly valuable, and I’ll certainly keep them in mind as I work on future projects.
I’m also planning to revisit my code and implement the changes you recommended. I believe these improvements will make a significant difference. Thanks again for your help and for contributing to my growth in this area!
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