Design comparison
Solution retrospective
Quick coding, so not the most effective way to implement everything. I had not code for a while so don't mind reminding me things i forgot, especialy on the html/css structure.
I struggled with the background so it don't creat a scroll bar, so i did what i beleive to be a dirty trick with overflow-x: hidden
I tried to implement the 5 stars with background repeat but i couldn't find out to space out the stars without going on photoshop and resize the original icon. So i just implemented 5 <img>
x3 times, which i think is not ideal for cache purpose.
Tell me if there is a better way.
Thx a lot
Community feedback
- @pikapikamartPosted about 3 years ago
Hey, awesome work on this one. Layout in desktop looks great, it is responsive, however you you are using too much screen-size for the mobile layout. Right now, at point 1200px going down, the mobile layout is already being shown, desktop layout could have used more of those screen-size. The mobile layout however looks great as well.
Some other suggestions would be:
- Avoid using
height: 100vh
on a large container especially thebody
tag, this makes the element's height limited to only remaining screen/viewport's height, try inspecting your layout in dev tools at the bottom, hover on thebody
tag, you will notice that it doesn't capture the whole content. Instead, usemin-height: 100vh
this takes full height and will expand if it needs to. - For this one, I wouldn't use
header
I would just usemain
to wrap the whole content since the contents is just one component or a section. - The text after the
h1
should be usingp
tag, use meaningful elements on any content, you could wrap thespan
inside thep
tag or just usep
tag on it. - Each
img
of the stars should be usingalt=""
attribute as well asaria-hidden="true"
on it, since it is only decorative image, hide it. Also, always remember to include thealt
attribute, don't forget to include it. - Again the text for the ratings should be wrapped inside meaningful element like
p
tag. - Each person's image should be using their name as the
alt
value likealt="Colton Smith"
, if an person's name and image is present at a page, always use their name as thealt
value. - Great that you use heading tag on the person's name, but the heading level is wrong. If you use
h3
make sure thath1, h2
are present as well, use heading tag with their level incrementally by 1.
Aside from those, site looks great.
Marked as helpful1@Nam-HaiPosted about 3 years ago@pikamart Thank you very much. It is exactly the respond i was searching for. A reminder of every little detail i might have forgot over the summer.
0 - Avoid using
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