NFT Preview card solution using flex box

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, nice work on this one. The overall layout looks great and it responds well on a very small mobile size as well.

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

  • Avoid using 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.
  • Always have a main element to wrap the main content of the site. For this use main tag on the .nft selector.
  • 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:> (just to say that I copied this line on my just previous review on this same challenge)
  • The preview-icon img is only being used as interactive on this one. 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.
  • Also, when using img tag, you don't need to add words that relates to "graphic" such as "icon" and others, since img is already an image so no need to describe it as one.
  • If you somehow go to dev tools at the bottom, you will see that the font-size has shrunk down. Upon seeing the css, do not use vh unit on the font-size as it is not consistent because lots of users have different screen-size. Use instead rem so that it will be consistent.
  • The h1 in here is being treated as an interactive since it has this :hover state right, therefor use an a tag to wrap the nft name along with the h1 wrapping it:

  <h1>
    <a href=""></a>
  </h1>
  

  • The text below the h1 is not really a heading tag at all and just a simple text so using a p tag would be really nice.
  • Also, those eth text and the "3 days left" are not heading tags. Only use heading tags on text-content where you think that the text gives information on what a section or part of the layout will contain.
  • The clock-icon is decorative as well so using the method above to hide it would be really nice.
  • The person's image could be using the person's name as the alt value since it is already present like alt="Jules Wyvern".
  • Change the span that wraps the person's name into a tag since it is an interactive.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Carlos Pimentel•10

  @PinkyToe87Posted almost 3 years ago

  @pikapikamart - First, thank you for the in-depth feedback! I've made some changes as per your suggestions:

  1. The body is now set to min-height 100vh.
  2. My nft div is now a main tag.
  3. I keep this in mind for future challenges: button vs a tag
  4. I set the alt="" and aria-hidden="true" for the preview-icon img
  5. The alt="Icon View" has been set to alt=""
  6. This one is confusing to me. Will using rem help the font be responsive?
  7. I wrapped the h1 in an a tag
  8. Text below h1 has been changed to p tag with class name
  9. Changed h4 to p tag with class
  10. On clock img, I set alt="" & aria-hidden="true"
  11. Alt has been changed to person's name
  12. I changed the span to an a tag

  Once again, thank you for the feedback.

  Thank you,

  Carlos

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  @PinkyToe87 Hey, glad that you found my review useful^^

  For your number 6 question ( the only question from the reply ), yes, using rem unit will make your layout more responsive since rem unit is relative to the user's preferred font-size on their browser. For example, I had set my browser's setting to using a large font-size, meaning, a site that uses rem unit will scale depending on the font-size of what I set. Using px on the other hand will not scale since px unit is a fixed unit and will not change no matter what.

  Marked as helpful

  0
• 

  Michael B•210

  @MrBeamerPosted almost 3 years ago

  @ PinkyToe87 looks good and you already made it responsive, but you centered the card in the mid, so you have to drag in all the way to left - to see the effect :)

  0