Used BEM on my CSS naming convention and applied Image overlay Icon.

Community feedback

• 

  Raymart Pamplona•16,040

  @pikapikamart

  Posted over 3 years ago

  Hey, really nice work on this one. The overall layout looks great I think, it just has this outer background-color and in the design it doesn't have that.

  David Turner already gave feedback on this one, just going to add some suggestions as well:

  • Remove the overflow: hidden on the body tag. Doing this will just prevent a user from scrolling on the site, imagine that there are lots of content in here, applying this will prevent them scrolling on those. Only use it on a specific component that needs to hide its overflowed content.
  • Always have a main element to wrap the main content of the site. For this use main tag on the .container selector.
  • On this one, since the image is being treated as an interactive component since there is a :hover state, then make sure that there is an interactive element used in there as well or wrapping the image. It could either be button or a tag and use depending on what you think will happen if clicking the image. If clicking the image pop-up something that the user can see the full nft, then go with button but if clicking it redirects user in another page to see the full nft, then go with a tag.
  • Also, for an a tag to work properly, always include the href attribute.
  • For now, you can use h1 on the nft name but have a look about sr-only h1 tag.
  • Those eth-icon and the clock-icon right now are only acting as decorative images. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag. Although the eth-icon could be using an alt I would still remove it since like I said, on a real site there would be bunch of nft and we don't want the user to traverse each eth-icon img.
  • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span to wrap the text.
  • Person's image could use their name as the alt value since it is already present and it makes sense to do so since they are being featured as the creator of the nft.
  • When using img tag, you don't need to add words that relates to "graphic" such as "image" and others, since img is already an image so no need to describe it as one.

  Aside from those, great job again on this one.

  28

  Marked as helpful
• 

  P

  David Turner•4,130

  @brodiewebdt

  Posted over 3 years ago

  This looks very good. Spacing and alignment look good. Hover effect are working. Great job.

  Wrap your container div in a Main tag and change the H3 to an H1 and it will clear the accessibility warnings.

  Download AXE DevTools and you can clear accessibility warnings while you code. https://www.deque.com/axe/devtools/

  Hope this helps.

  04

  Marked as helpful