NFT Preview Card component

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, awesome work on this one. The overall layout of the site looks great.

  MikevPeeren already gave a feedback on this one, just going to add some suggestion as well:

  • Avoid using height: 100% or height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
  • I just noticed a usage of position: absolute on the "inner-container selector. Remember that when using position: absolute on an element, it goes out of the flow of the site. For this one, you don't really need to use that to center the layout and you can just remove the position: absolute on the .inner-container then just add min-height: 100% on the main tag.
  • Since you are treating the image as an interactive component, make sure that there will be an interactive element beside or nesting the image, though I would prefer to the element just beside. I haven't tackled this challenge yet so I can't provide an exact reference, but just always remember that interactive elements like button, a tag etc, should be used when there is an interactive component.
  • For the nft image, you can use the nft name as the image text like alt="Equilibrium" since it is already present so why not make use of it.
  • For the image overlay, since it is decorative image, use add an extra aria-hidden="true" on it so that it won't be picked up by screen-readers.
  • You can use h1 for the equilibrium name for now but it would be better if the h1 is used a screen-reader only h1. Have a search about that one, but yeah, you can use h1 for the name for now.
  • Also, the nft name is being treated as interactive since it has a hover state, wrap the nft name inside a a tag since it will be page link on a real site.
  • You can use alt="ethereum" on the ethereum icon since it is a real image and is a currency on crypto.
  • Add an extra aria-hidden="true" on the clock-icon.
  • Person's image should be using the person's name as the alt value like alt="Victor Crest". Components like this where a person's information is presented, make sure to use their name as the person's img alt value.
  • Lastly, wrap the person's name inside of a a tag since it is treated as interactive as well.

  Aside from those, great job again on this one.

  Marked as helpful

  0

  ---

  

  4gotpwd•40

  @4gotpwdPosted almost 3 years ago

  @pikapikamart wow! thats a lot of pointers there! I really appreciate it. I will take sometime to understand each of it and apply it on my next solution. I hope to see you around again with one of my solutions! thank you!

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  @4gotpwd Great that you found my review useful:>

  Looking forwards for your upcoming challenge submissions here at FEM!

  0
• 

  MikevPeeren•2,100

  @MikevPeerenPosted almost 3 years ago

  Hey 👋

  Try to add some hover transitions to your hovers, as for the hover image it looks good although the position isn't centered so try to fix that, for example with a transform.

  Marked as helpful

  0

  ---

  

  4gotpwd•40

  @4gotpwdPosted almost 3 years ago

  @MikevPeeren I have made the changes based on your feedback. Would you mind if you check? Cause the design comparison bothers me. It looks good on the actual but on the design comparison, something is breaking. :(

  0

  

  MikevPeeren•2,100

  @MikevPeerenPosted almost 3 years ago

  @4gotpwd looks good 👍

  Maybe make a new screenshot for your new changes to solve the difference ?

  Marked as helpful

  0