NFT preview component page

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

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

  Mohamed Abdessamed already gave really nice feedback on this one, just going to add some suggestions as well:

  • If you try to zoom out on your screen, you will notice that the layout is not being centered properly vertically. For this one, using only the margin from the .card is not enough. Instead, remove those margin, margin-top, margin-bottom on the .card and on the body tag, add these stylings:

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

  This will ensure that the layout will stay center at every screen-size.

  • For this one, it would be nice to use main and footer. Change the .card to using main and the .attribution to be using footer. This way, the contents will be nested inside of a landmark element/s.
  • Avoid using id to target and style an element since it is a bad practice due to css specificity. Instead, just use class to target element.
  • On this, since the nft image is being treated as interactive component because it has a :hover state which shows an overlay of a preview-icon, meaning there should be an interactive element being used in here. if you think that clicking the image shows a pop-up where the user can see the full nft, then go with button. But if you think that clicking the image will take the user in another page to see the full nft, then go with a tag. I haven't tackled this one out yet so I don't have any reference to give but I hope you get the gist from this one:>
  • When using img tag, you don't need to add words that relates to "graphic" such as "pciture" and others, since img is already an image so no need to describe it as one.
  • You can use the nft name as the nft img alt value since it is already present like alt="nft Equilibrium #3429". This is still ambiguous since it is not really depicting what the nft image looks like, but still, an nft is to be seen so adding an alt will be better.
  • Same for the eth-icon, remove the word "icon".
  • The clock-icon on this one is only a decorative image. 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.
  • Only use descriptive alt on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
  • The person's image could be using the person's name as the alt value, you could go with alt="Jules Wyvern".
  • On your .iconline selector, place the text-content inside the p tag. The p tag will be nesting the person's name as well and remove the span and just use a tag:

  <p>
    <a href="">Jules Wyvern</a>
  </p>
  

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Arnold Ho•20

  @arnoldboy123Posted almost 3 years ago

  @pikapikamart Thanks so much for your detailed response! This is really helpful!!!!

  1
• 

  Mohamed Abdessamed•140

  @Brivan-26Posted almost 3 years ago

  To get the active state image, I can suggest you the following structure: -You will have a div (by name of header as an example) that contains an image(the NFT image) and another div by name active(inside this last div, you will have an eye icon). -In CSS: 1/target the header dive and add position: relative; 2/target the active div and make its position absolute, opacity set it as 0 by default, and when this div is hovered, make opacity: 1; to Center the icon, you can either use flexbox trick or top,left,right,bottom properties as we set position absoule

  Marked as helpful

  1

  ---

  

  Arnold Ho•20

  @arnoldboy123Posted almost 3 years ago

  @Brivan-26 Thanks! I have now changed it according to your suggestion and some other solutions and it now works!

  0