Design comparison
Solution retrospective
This was my first ever (small) coding project, my question is simple and i hope you all can tell me what i could have done better so i can become a better web developer.
- Are there any best practices i should have used?
- Did i do it the easiest way or are there better / faster ways to tackle this project?
Community feedback
- @DreamleafPosted 11 months ago
Pretty clean code for a beginner, well done!
Two things I can spot in your code that would improve things that may not be obvious.
For the QR image, be specific about what the image is doing... eg. "QR Code to frontendmentor.io" instead of just "QR Code". This will be more informative for users of screen readers and tell them exactly what will happen.
Secondly, you have used 100vh on the body class. This isn't wrong, but you may sometimes get unintended results on mobile viewports. Do a search and read about the alternative values of SVH and DVH - basically, just using VH includes UI elements such as the address bar - the alternative can ignore these and just focus on the DOM part of the view.
Great work though, I look forward to seeing you tackle more challenges.
Marked as helpful1@S02KPosted 11 months ago@Dreamleaf
Thank you for the helpful feedback i have now changed the ALT text i will also read up on SVH and DVH so i don't run into mobile issues later on!
1 - @Islandstone89Posted 11 months ago
Hi there. I agree with the comment above, your code is quite good for a beginner. You have a
<main>
, which most beginners don't include, and you're using Custom Properties. The card is nicely centered using Flexbox, and you are usingpadding
to create the space between the image and the edge of the card - many tend to usemargin
on the image, instead.Here are my tips on how to make your solution even better. Hope it helps :)
HTML:
-
.attribution
should be a<footer>
. -
Footer text needs to be wrapped in a
<p>
.
CSS:
-
Performance-wise, it's better to link fonts in the
<head>
of the HTML then using@import
. -
It's good practice to include a CSS Reset at the top.
-
The
body
should have amin-height
instead ofheight
, this allows the content to grow. -
Font-size must never be in px. Use rem instead.
-
Remove the fixed width. You rarely want to set fixed dimensions, as this easily creates issues with responsiveness.
-
Add a
max-width
of around 20rem on the card, to prevent it from getting too wide on larger screens. -
Since all text is centered, I would set
text-align: center
on thebody
, and remove it elsewhere. -
The image should have
display: block
, and instead ofwidth
usemax-width: 100%
.
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