Design comparison
Solution retrospective
I generally felt it was an easy project—my first project on front-end mentors. I feel a bit unsure about using <article>, which particular cases do use semantics tags such as main and article are strongly recommended over traditional divs.
In my CSS, I just did what worked. There are best practices I might not have adhered to and I'm willing to learn through the next challenges.
Community feedback
- @correlucasPosted about 2 years ago
Hello Ian Rioba, congratulations for your solution!
Your solution is working really good and I've not much to say about it in general.
But if you want to clean your code, even if is already working, you can build this component using a single div to hold all the content
(img, h1 and p)
and clean all the other divs.<body> <article> <img> <h1></h1> <p></p> </article> </body>
Note that also for a clean css in this case you don't need any class, you can use each elements selectors to manage it in the css sheet, for example
article, img, h1 and
can be used with no need for classes.Doing that you'll have a lean and clean solution and write 20% less code than the current html/css sheet.
Hope it helps and happy coding!
0@Rioba-IanPosted about 2 years ago@correlucas Thank you for your review. You are absolutely right. I shouldn't introduce a div to hold the
<h1> and <p> inside the article. I ended up increasing CSS. I will review and make those changes.0 - @PhoenixDev22Posted about 2 years ago
Hello Ian Rioba,
Congratulation on completing your first frontend mentor challenge. Your solution looks great. I have some suggestions regarding your solution:
-
You should use
<footer>
landmark for the attribution. HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology. -
Images must have alt attribute.The alternate text should indicate where the Qr code navigate the user : like
QR code to frontend mentor
. -
Page should contain
<h1>
. In this challenge , as it’s supposed to be a part of a whole page, you may use<h1>
withsr-only
class hidden visually and present for assistive tech users.
- Adding
rel="noopener"
orrel="noreferrer"
totarget="_blank"
links. When you link to a page on another site usingtarget=”_blank”
attribute , you can expose your site to performance and security issues.
Aside these , Excellent wok! hopefully this feedback helps.
0@Rioba-IanPosted about 2 years ago@PhoenixDev22 Thank you for these highlighting points. I have some important pieces out of these as they've also been reported in the accessibility issues. I'll work on them.
About the rel="noopener", that's something that I've actually met today. Thank you.
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