@pikapikamart
Posted
Hey, really nice work on this one. The desktop layout looks really great, the site is responsive as well and I like that layout when you go tablet size, looks really nice to be honest. Mobile state looks great as well.
Here are some suggestions for the site:
- For this one, you should replace the
article
tag into usingmain
tag since amain
tag is needed for a page so that it will be easy for user to know the main-content since they could just traverse the landmark. - Inside the
.grid
selector, you could change eachsection
to justdiv
.section
tag is not that informative as landmark element unless you usearia-labelledby
on it so that screen-reader will read an extra information about the landmark.div
would be fine since traversing by the heading tag is the same when you just use a plainsection
tag. - I noticed that each of the
.card
selector has a smallheight
, you can see that if you hover when inspecting the layout. For this one, theheight
is not really needed on each.card
. What you can do is that, each icon on the top part of the.card
could just be used asbackground-image
for each.card
selector, this way you could just add apadding-top
on it to simulate the dark-ish container below it to be pushed below. - If you insist on using
img
tag, then usealt=""
and addaria-hidden="true"
on it since it is only a decorative image. - Since you used
button
on the 3 dots, you should have eitheraria-label
attribute orsr-only
text inside it which defines what thebutton
does. - The 3 dots
img
is decorative as well, hide it using the method above. - Do not remove the
outline
styling. If you did, always include a visual-indicator on the:focus-visible
for those interactive elements like thebutton
a
tag and others. - Since you are using a
button
tag on this one, you could use aul
tag to wrap thosebutton
since those are "list" of selections. - Also, since
button
alone is not informative specially when a user toggles it using a screen-reader, you should have used either anaria-live
element that will announce whether a specificbutton
has been toggled. Another approach would be to use something likearia-pressed
on eachbutton
, you set the attribute totrue
if abutton
is pressed and for others as well. - Lastly, the
.attribution
should be using afooter
tag so that it will be its own landmark element.
Aside from those, great job again on this one.
Marked as helpful
@dragoshcode
Posted
@pikapikamart Thanks a lot, that's kind of a professional feedback!