Design comparison
Community feedback
- @vanzasetiaPosted over 2 years ago
Hello, KurtJF! π
Congratulations on completing your first Frontend Mentor challenge! π
Regarding your questions:
- It's possible to finish this challenge without using any
div
elements. I suggest removing thediv
with the class of.card
. Also, it's not a necessary thing to wrap the text elements (h1
andp
) with adiv
. I suggest making those elements as the child elements of themain
element. For the attribution, you can swap thediv
with afooter
instead. - I think you have done a great job on using CSS custom properties. But, it's hard to tell how well it is since the project is a small project though. π
Some suggestions from me.
- The alternative text for the QR code can be improved. Try to tell the users about the QR code. By the way, have you tried to scan the QR code?
- I recommend adding
width
andheight
attributes to each image element. This way, the browser knows how much space the image requires before it can be fully loaded. As a result, it would optimize CLS (Cumulative Layout Shift). As a side note, it is not the same aswidth
andheight
CSS properties. So, make sure you learn how to use them.
That's it! I hope you find this useful! π
Marked as helpful1@KurtJFPosted over 2 years ago@vanzasetia Thank you for your detailed response regarding my questions! It really helped a lot, Also I have updated my solution.
If you don't mind me asking, How can I tell if the
div
is not needed?Again thank you very much for the comments and I will keep them in mind when tackling the other challenges. π
0@vanzasetiaPosted over 2 years ago@KurtJF Happy to hear that was helpful! π
In this case, you don't need any
div
because you can use themain
element as the container of the elements inside it. Also, theattribution
should be afooter
to fix the issue that has been reported.In general, you want to keep the HTML markup as simple as possible. So, I recommend trying to write as minimum HTML as possible. After that, if it needs
div
to wrap the elements in order to create two-column layouts then use adiv
.The point is to use
div
when it is needed. πBy the way, good job with the update! The HTML markup looking good to me. π
Marked as helpful0 - It's possible to finish this challenge without using any
- @correlucasPosted over 2 years ago
πΎHello again Kurt, congratulations for your new solution!
Great great great solution you've here, I liked a lots that you html structure is
super clean
this is amazing Kurt. If you want to clean even more you code you can remove all the classes and use the direct selector for each element to manipulate the CSS, for example select everything withmain, img, h1 and p
.All you need to make this container responsive is to replace
width: 320px;
withmax-width: 320px;
.card-container { border-radius: 22px; margin: auto; background-color: var(--white); max-width: 320px; padding: 16px; }
π I hope this helps you and happy coding!
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