Raymart Pamplona• 16,090
@pikapikamart
Posted
Hey, awesome work on this one. Desktop layout looks really great and the mobile state looks really great as well.
I don't see any problems on the image though, have you already finished it?
Some suggestions would be:
- Remove the
overflow: hidden
on themain
tag. Try inspecting the layout in dev tools at the bottom adjust the height of the dev tools, you will notice you can't scroll on the layout because of that property. - Also, avoid using
height: 100vh
on a large container, it is a bad practice since it limits the element's height based on the remaining screen/viewport's height. But, I tried removing it on this one and the layout really break. Have a go on this again without using that,min-height: 100vh
is always better. - Add
aria-hidden="true"
to those images that are decorative so that they will be totally hidden for screen-reader users. - Using
div
for the accordion is not really great since it makes the component not accessible. Right now, only mouse can trigger it and it is excluding lots of users. Instead, usedetails
element. It is suited for accordions and it is accessible.
Aside from those, great job again on this one.
Marked as helpful
1
Trần Thanh Tâm• 235
@TripleT511
Posted
@pikamart thank you.
1