@pikapikamart
Posted
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 helpful
@Nam-Hai
Posted
@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.