NFT card component

Community feedback

• 

  Raymart Pamplona•16,040

  @pikapikamart

  Posted over 3 years ago

  Hey, nice work on this one. Layout in general looks great, just needed for it to be centered vertically. For responsiveness, it responds but when you go to like 374px, the screen-starts to hide the content and creates horizontal scrollbar.

  Leonardo de Souza Nunes already gave great feedback on this one, just going to add some suggestions as well:

  • It would be great to have a base styling of this:

  html {
    box-sizing: border-box;
    font-size: 100%;
  }
  
  body {
   margin: 0;
  }
  
  *,
  *::before,
  *::after {
    box-sizing: inherit
  }
  

  This way, handling an element specially its size will be easier because of the box-sizing

  • Always have a main element to wrap the main content of the site. For this use main tag on the .main selector.
  • Using margin alone is not consistent when centering a layout, especially when you need to center it vertically. On this one, instead of that , remove the margin on the .main and on the body tag, you could add these stylings:

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

  If you don't know these stylings, have a search about flexbox^^

  • Since the nft image name is already present on the site, it would be nice to use the name as the img tag's alt value like alt="Equilibrium".
  • Also for the hover-overlay. As of now, I haven't seen a solution that uses interactive element on this which should be present. I don't have a reference on this since I haven't made this challenge yet but let me just give a quick idea on approaching element in general.

  When you are doing a component, let's say this one, the image. Since on the design, if you are hovering on it, it would create an overlay and like have a preview-icon right. When an component is being treated as an interactive component, always make sure that you are using an interactive element like button or a tag. Because lots of submission, they are only adding the :hover state on the img tag but there is no interactive element being used. For this, imagine if you click the img, what would happen? Does clicking it would take a user in another page to view the full nft, if yes, then a tag will be better to use. But if clicking the img will only like show a modal/pop-up to see the full image and not redirect a user in another page, then using button will be your choice.

  • Also for the nft name, since you are treating it as interactive as well, make sure that it uses interactive element. You could use a tag to wrap the nft name:

  <h1>
   <a href="#" >Equilibrium #3429</a>
  </h1>
  

  This way, it is proper.

  • Change those 3 section tags on the eth, clock and the person part. section tags is not informative as a landmark unless they are used with aria-labelledby attribute. You could use section if it already have a visible heading tag. But for this, using a div would be fine.
  • When you think an img is decorative, adding an extra aria-hidden="true" attribute on the img would be really nice, along with the alt="'. This way, screen-reader won't picked those up since they are being hidden from it.
  • Person's image should be using the person's name as the alt value like alt="Jules Wyvern". Components like this where a person's information is presented, make sure to use their name as the person's img alt value.
  • Also, since the person's name is again, being treated as interactive, adding using a tag rather than span will be much better.

  Aside from those , great job again on this one. Let me know if you have queries and see if I can help with it.

  10

  Marked as helpful
• 

  Leonardo de Souza Nunes•100

  @LeoSouzaNunes

  Posted over 3 years ago

  Instead of declare a image tag in your index.html you could set a div and place the image with the background-image property in your CSS file. I believe that with this approach would be easier to set another div inside of the main image and make the same process but this time with :hover{background-image:}.

  00

  Marked as helpful