Design comparison
Solution retrospective
Would love to have some feedback on the markup and styling.
What would you do differently, if it was yours?
What would you ask me to change, if you were my collegue, who will work with it after me?
Thak you all for your time!
Community feedback
- @MelvinAguilarPosted 11 months ago
Hello there ๐. Good job on completing the challenge !
I have some suggestions about your code that might interest you.
div.product
shouldn't have a width because adiv
is a block-level element and takes up 100% width by default. Additionally, you should usemin-height: 100vh
in that selector to prevent it from becoming too short on small screens. The.page
selector has unnecessary height (min-height), as the inner div overwrites that height with100vh
.
- Use the
<footer>
tag to wrap the footer of the page instead of the<div class="attribution">
. The<footer>
element contains information about the author of the page, the copyright, and other legal information.
- The
<h1>
is the most important heading on the page, In this challenge the perfumer's name can be considered like the title of the page, so it should be the<h1>
I hope you find it useful! ๐ Above all, the solution you submitted is great!
Happy coding!
Marked as helpful0 - @danielmrz-devPosted 11 months ago
Hello Eugenia!
Your solution looks excellent!
I just have one suggestion:
- I noticed that you used 3 different tags (main, div and article) to wrap the main content.
Unless you have a specific reason to do that, it's not necessary.
The tag
article
would make more sense if the card was part of a bigger website (and it certainly would in real world), but here it is all we have on the screen.The tag
div
is good, but it has no semantic value.The tag
main
is the one for this case, since the main content goes inside of it.I hope it helps!
Other than that, you did an excellent job!
Marked as helpful0@EugeniaAntonovaPosted 11 months ago@danielmrz-dev Thank you! That truly helps. I have used all those wrappers for some reasons in my head, but thinking was wrong, as i see now:
- main is used, because all the pages need it
- div is used to center the content and to color the page (as if the rest of the page was some other color or so) -article is used, because it looks as if it might be seperately reused in different places
Oppinion of such an experienced person is extremly valuable!
1
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