Design comparison
Solution retrospective
Any feedback would be great
Community feedback
- @correlucasPosted about 2 years ago
πΎHello @fsuropaty, congratulations for your new solution!
You've done a really good work here using semantic tags and putting everything together, I've only one suggestion about the responsiveness and the use of the
id
.I looked your live site and thecomponent
is not RESPONSIVE yet, this is due thefixed width
you've applied to the container. Look bothwidth
andmax-width
the main difference between these properties is that the first(width) is fixed and the second(max-width) isflexible
, for example, a component withwidth: 320px
will not grow or shrink because the size will be ever the same, but a container withmax-width: 320px
ormin-width: 320px
can grow or contract depending of the property you've set for the container. So if you want a responsive block element, never usewidth
choose ormin-width
ormax-width
.It is not advisable to use IDs as CSS selectors because if another element in the page uses the same/similar style, you would have to write the same CSS again. Even if you don't have more than one element with that style right now, it might come later.
βοΈ I hope this helps you and happy coding!
Marked as helpful1 - @AdrianoEscarabotePosted about 2 years ago
Hi @fsuropaty, how are you?
I really liked your project, the responsiveness of the layout was very good, but I have some tips that I think you will like:
1- Avoid using classes or ids when there is only one element of the same type in the document.
2- You could have put
min-height: 100vh;
inside the body. Instead of putting a 95vh in the containerThe rest is really good! Hope it helps...π
Marked as helpful1@fsuropatyPosted about 2 years ago@AdrianoEscarabote Thank you very much for your feedback
1 - @vanzasetiaPosted about 2 years ago
Hi there! π
Congratulations on finishing this challenge! π
Here are some recommendations for improvements.
- Put the
footer
outside themain
landmark. Otherwise, thefooter
will act like adiv
instead of a landmark element. - Never limit the height of the
body
element. It will not allow the users to scroll the page if the page content needs moreheight
. You can see the issue by looking at the site on a mobile landscape view. So, my recommendation is to usemin-height
instead. - Alternative text should not be hyphenated. It will be read by screenreader so it should be read like normal text or human-readable text. Also, I suggest improving the alternative text by providing more information about the QR code.
Hope this helps! π
Marked as helpful0@vanzasetiaPosted about 2 years ago@fsuropaty
One more important suggestion is to use single class selectors for styling whenever possible instead of
id
.id
has high specificity which can lead to a lot of issues on the larger project. It's best to keep the CSS specificity as low and flat as possible. π1 - Put the
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