Design comparison
Solution retrospective
hi it's my first project. Any feedback or any kind of improvements that I can make will be very much appreciated.
Community feedback
- @keyztrokeePosted about 2 years ago
Hi, Rico! Very cool solution, it really looks great :)
I noticed three things that you might want to consider:
- Change the h1 color to var(--clr-dark-blue) because it is not black.
2.Use <main> instead of <div> to wrap the card container. This way you show that this is the main block of content and also replace the div with a semantic tag.
3.Reduce your code by removing unnecessary elements. The HTML structure is working but you can reduce at least 20% of your code by cleaning the unnecessary elements, you start cleaning it by removing some unnecessary <div>. For this solution you wrap everything inside a single block of content using <div> or <main> (better option for accessibility) and put inside the whole content <img> / <h1> and <p>.
Other than that, really great job! :)
✌️ I hope this helps you and happy coding!
Marked as helpful1@keyztrokeePosted about 2 years ago@correlucas Hi, I just copied it from your suggestion to my work and I think it will help him too. Great help by the way, thank you very much!
0@keyztrokeePosted about 2 years ago@correlucas Can you give feedback on my last work too? I would love to see what I can do better with it. Please consider taking a look at it if you have time. Thank you very much!
0@ShuliiiPosted about 2 years ago@keyztrokee thankyou so much for the feedback. much appreciated.
0 - @correlucasPosted about 2 years ago
👾Hi @Shuliii, congratulations on your solution!👋 Welcome to the Frontend Mentor Coding Community!
Great solution and a great start! From what I saw you’re on the right track. I’ve few suggestions for you that you can consider adding to your code:
1.Think about using relative units as
remoreminstead ofpxto improve your performance by resizing fonts between different screens and devices. Anyhow, if we want a more accessible website, then we should use rem instead of px. REM does not just apply to font size, but to all sizes as well.2.Add the website favicon inserting the svg image inside the
<head>.<link rel="icon" type="image/x-icon" href="./images/favicon-32x32.png">3.
IMPROVE YOUR WORKFLOWusing VSCODE you can code your whole page usingpxand then in the end use a plugin called px to rem here's the link → https://marketplace.visualstudio.com/items?itemName=sainoba.px-to-rem to do the automatic conversion or use this website https://pixelsconverter.com/px-to-rem4.
Keep the image responsive. To manage the image size, you don’t need to define thewidthandheighttogether, if you do it with different values this will make the image lose proportions, to keep the image responsive and respect the container size useimg { display: block; max-width: 100%}this way the image resize with the container whatever its size.Here's my solution for this challenge if you wants to see how I build it: https://www.frontendmentor.io/solutions/qr-code-component-vanilla-cs-js-darklight-mode-nS2aOYYsJR
✌️ I hope this helps you and happy coding!
Marked as helpful0
Please log in to post a comment
Log in with GitHub
Join 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