@pikapikamart
Posted
Hey, awesome work on this one. The overall layout of the site I think looks fine, just needed for the box-shadow
to be smoother, currently it has this solid shadow.
Others already gave their feedback on this one which is nice, just going to add some suggestions as well:
- Avoid using
height: 100%
orheight: 100vh
on a large container like thebody
as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to. - Always have a
main
element to wrap the main content of the site. For this usemain
tag on the.card-underlay1
selector. - For the
.attribution
, it would be nice to replace thediv
with afooter
tag so that it will be its own landmark element. - For the image's interactivity, since it is treated as an interactive component because of the hover effect right, it would be really great to have an interactive element alongside with it. You can use
button
if you think clicking the image would show a pop-up where the user can see the full nft or ana
tag if you think clicking the image would take a user in another page to view the full nft. I haven't made this one yet so I can't provide a reference on it, sorry for this one, but I hope you kind of get the gist of what I said:> - The overlay-image when hovering on the nft image should be hidden since it is only a decorative image. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if you are usingsvg
instead ofimg
tag. - For the nft name, again it is being treated as an interactive component right, nesting the
h1
test inside of ana
tag would be really nice:
h1
a: text in here
- Also just saw the
alt
for the clock-icon. When usingimg
tag, you don't need to add words that relates to "graphic" such as "icon" and others, sinceimg
is already an image so no need to describe it as one. - On an image, using
img
tag, if you think that the image is meaningful and really adds content to the site, then use a meaningfulalt
value. But if the image is just acting decoration, then hide it using the method I mentioned above. - For the person's image, you can just use the person's name as the only
alt
value and you can remove the other text inside it. - Lastly, the person's name is interactive as well. Instead of using bold tag (
b
) usea
tag to wrap the person's name.
Aside from those, great job again on this one.
Marked as helpful
@lmonteiro18
Posted
@pikapikamart I really appreciate all the suggestions you made! There are somethings that really are basic and i forgot about them somehow (like wraping things on landmarks) and I will pay more attention from now on! Again, thank you a lot for helping me improve!