NFT preview card component

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, awesome work on this one. The overall layout I think looks great.

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

  • Don't use position: absolute a very large element. If you inspect your layout, hover on either html or body tag, you will notice that it has no height since its element is being absolute. Since you are just using this to center the layout, it would be much better to do it this way. But first, remove these stylings on the .container selector:

  position
  top
  left
  transform
  

  I also saw that the .card is using this stylings, you can remove that as well. Typically, we only want to absolute something when we are properly controlling the element. For this one, you don't really need that since a flexbox parent could do it properly. You could add these on the body tag:

  align-items: center;
  display: flex;
  justify-content: center;
  min-height: 100vh;
  

  • The .container should be using main like what David had said, because main tag is landmark element that helps/guides user to navigate content properly using screen-reader and just semantic markup in general.
  • For this one, since the nft image is being treated as interactive component, there should be an interactive element that nest or sits inside the container. For this, you are not using an img for the nft image ( could use img ), div in general is not good for being interactive since it won't be toggled by other peripherals. Use either a tag or button on this, it could nest the nft image.
  • The preview-icon on this one is only acting as decorative images. Decorative images are just images that doesn't contribute to the overall content of the site. They 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.
  • On this, the nft name could be using an h1. But if you don't want to use h1 on it, change it to h2 so that you won't be skipping a heading tag level. You may remember, an h1 is needed on the page. Since there are no visible text that are suitable to be h1, the h1 would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, the h1 would have like sr-only class and the text-content should describe what is the main-content is all about. The h1 could be placed as the first text inside the main.Have a look at this example snippet that I have about screen-reader only h1. Comments are already included in here.
  • The nft name is treated interactive as well. Wrap the inner text of the heading tag by a tag.
  • The eth-icon and the clock-icon could be just hidden since on a real site, there would be bunch of nft card with the same content and there would be plenty of these icons in there as well. You don't want the user to traverse all these repetitive images. Use the method above.
  • 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 img could use the person's name since it is already included.
  • Use a tag instead of span to wrap the person's name since span is not intended to be interactive.

  Aside from those, great job again on this one.

  Marked as helpful

  0

  ---

  

  Guilherme Trigo Zin•90

  @TrigoZinPosted almost 3 years ago

  @pikapikamart Oh, thanks for the help!

  I work on it to be better, thanks a lot! ♥

  1
• 

  P

  David Turner•4,150

  @brodiewebdtPosted almost 3 years ago

  Looks very good. The alignment and spacing look great. Hover effects all work right.

  Wrap your container div in a Main tag and change the Equilibrium text to an H1 and it will clear the accessibility warnings. You will have to re-style the H1 to look like it does now.

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

  Hope this helps.

  Marked as helpful

  0