@pikapikamart
Posted
Hey, awesome work on this one. Desktop layout looks really great, it is responsive and the mobile state looks great as well.
Carlos already gave feedback on this one, just going to add some suggestions as well:
- Don't use
overflow: hidden
on thebody
tag, try to inspect the layout while the styling is active, you will notice that you can't scroll because of that style props. - Always have a
main
element to wrap the main content of your page. On this one, the.card
should be using themain
instead ofarticle
. - For those decorative images that are using
alt=""
, add an extraaria-hidden="true"
on it so that it will be totally hidden for screen-reader users. - For those accordion, using
a
tag is not really intended for those because you only usea
tag for links and that is not a link at all. Instead , use thedetails
element for the accordion as they are intended to be used that way and it is accessible right from the start so you won't have to manipulate attributes of the element. - The arrow-icon should be hidden since it is only decorative image. Only use descriptive
alt
for images that are meaningful and add content. Otherwise usealt=""
andaria-hidden="true"
on theimg
.
Aside from those, great job again on this one.
Marked as helpful
@CyrusKabir
Posted
@pikamart hello my dear friend . thanks for your great feedback the only reason for using <a> tag it was just for have open and close functionality with css you know using :target and this stuff Otherwise, I will follow all the points you said both in this project and in the next projects. Thank you again.