Design comparison
Solution retrospective
Hello everyone!
I hope you will tell me what you think about my development of this mini-project and what things could be changed and improved. It would be greatly appreciated!
Community feedback
- @VCaramesPosted about 2 years ago
Hey @SamCastle16, some suggestions to improve you code:
-
For this challenge you want to use the Image Element not the Background Image Property. By using the Background Image Property screen readers users have no idea where the image is to scan.
-
Once you fix the image implementation, you'll want to include an Alt text tag with them. Inside that Alt Tag Its needs to tell screen reader users what it is and where it will take them to when they scan it.
-
Change the
width
tomax-width
in your card container to make it responsive.
Happy Coding! 👻🎃
Marked as helpful2@SamCastle16Posted about 2 years ago@vcarames Hello! Thank you very much for your answer. I have already updated the project, improving the code with all your suggestions. I hope you review it again, if you find another detail or recommendation, I would appreciate it!
I implemented the QR code image inside the img tag.
I simplified the code by removing unnecessary divs and just using the basic html tags (main and footer). I also simplified the selectors in css, removing the classes to make the code cleaner.
I put the attribution outside the main element and inside the footer as you told me.
And I adjusted the responsive and it looks correct.
Thank you very much for the recommendations, they will make me grow a little more as a developer!
0 -
- @correlucasPosted about 2 years ago
👾Hi @SamCastle16, 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.Use relative units like
rem or em
instead ofpx
to have a better performance when your page content resizes on different screens and devices.REM
andEM
does not just apply to font size, but all sizes as well. To save your time you can code your whole page usingpx
and then in the end use a VsCode plugin calledpx to rem
to do the automatic conversion or use this website https://pixelsconverter.com/px-to-rem2.Your solution is great and the code is working, but the HTML structure can be reduced by removing unnecessary divs, all you need is a single
<main>
or<div>
to keep all the content inside, and nothing more. The ideal structure is thediv
and only the image, heading, and paragraph.Here’s one example to show can be cleaner this HTML structure:
<body> <main> <img src="./images/image-qr-code.png" alt="QR Code Frontend Mentor" > <h1>Improve your front-end skills by building projects</h1> <p>Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p> </main> </body>
To reduce the CSS you can use the direct selector for each element instead of using
class
this way you have a code even cleaner, for example, you can select everything using the direct selector for (img, h1, and p, main).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 helpful1@SamCastle16Posted about 2 years ago@correlucas Hello! Thank you very much for your answer. I have already updated the project, improving the code with all your suggestions. I hope you review it again, if you find another detail or recommendation, I would appreciate it!
I simplified the code by removing unnecessary divs and just using the basic html tags (main and footer). I also simplified the selectors in css, removing the classes to make the code cleaner.
Thanks for recommending the VSCODE "px to rem" extension. I will use it a lot!!!
0 - @vanzasetiaPosted about 2 years ago
Hi, Sandy! 👋
Congratulations on completing this challenge! 🎉
In addition to what @vcarames has said about the QR code image, only make the decorative images as background images. In this case, the QR code is the main content of the page. So, it should be on the HTML.
Then, put the attribution outside the
main
element. It should be live inside thefooter
.After that, I recommend using the
body
element as the container of the card. Also, usemin-height
instead ofheight
. You can see the issue by looking at the site on a mobile landscape view.Lastly, I suggest adding
rel="noopener"
to any anchor tags that havetarget="_blank"
. It helps protect users of legacy browsers. I suggest reading "Links to cross-origin destinations are unsafe" article to learn more about this.I hope this helps! Happy coding!
Marked as helpful1@SamCastle16Posted about 2 years ago@vanzasetia Hello! Thank you very much for your answer. I have already updated the project, improving the code with all your suggestions. I hope you review it again, if you find another detail or recommendation, I would appreciate it!
I implemented the QR code image inside the img tag.
I simplified the code by removing unnecessary divs and just using the basic html tags (main and footer). I also simplified the selectors in css, removing the classes to make the code cleaner.
I put the attribution outside the main element and inside the footer as you told me.
And I adjusted the responsive and it looks correct.
Thank you very much for the recommendations, they will make me grow a little more as a developer!
0@vanzasetiaPosted about 2 years ago@SamCastle16 You're welcome! 👋
There are two things that need to be done.
- First, add some
padding
on thebody
element to prevent the page content from touching the edges of the browser. - Second, add
rel="noopener"
to all links that havetarget="_blank"
.
Other than those two things, everything else is looking great to me. Good job! 👍
Marked as helpful1 - First, add some
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