NFT Preview Card Component

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  @Peteonthebeat

  You’re welcome and glad it was helpful.

  About the hover state , it’s mentioned that the user should be able to see the hover state on the interactive elements. When you check the design for the active states , there is a hover state on the equilibrium image.

  Happy coding!

  Marked as helpful

  0

  ---

  

  POTB•330

  @PeteonthebeatPosted over 2 years ago

  Hey @PhoenixDev22, Yes, I checked again, and indeed it's mentioned. It took me some time, but I figured out how to achieve some hover effect; I also updated the files.

  Once more, thank you so much for pointing that out and giving those pieces of advice!

  I really appreciate it!

  Sincere, POTB

  0
• 

  Sam•230

  @samd1aPosted over 2 years ago

  Hi POTB, congrats on finishing your challenge it looks great!

  I would remove the margin-right: 4em on your .eth-span class as its making the spans text wrap weirdly as the viewport width decreases.

  Other than that good work, keep coding!

  Marked as helpful

  0

  ---

  

  POTB•330

  @PeteonthebeatPosted over 2 years ago

  Hey @samd1a, Thanks for this comment! Yes, I forgot to remove the margin-right: 4em; it's absolutely redundant since I have display: flex, justify-content: space-between separating those. Now that I have updated the files, it should be on point.

  Regards, POTB

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hello @Peteonthebeat,

  Excellent work! Congratulation on completing this challenge. I have some suggestions regarding your solution if you don't mind: HTML

  • Since there's a :hover state on the image and means it's interactive, So there should be an interactive element around it. When you create a component that could be interacted with a user , always remember to include interactive elements like(button, textarea,input, ..) For this imagine what would happen when you click on the image, there are two possible ways:

  1: If clicking the image would show a popup where the user can see the full NFT, here you use <button>.

  2:If clicking the image would navigate the user to another page to see the NFT, here use <a>.

  • Also use <a> to wrap Jules Wyvern and Equilibrium #3429. the hover effects on the interactive elements are missing.

  • The link wrapping the equilibrium image (image-equilibrium ) should either have Sr-only text, an aria-label or alt text on the image that indicates where the link navigate the user.

  • For any decorative images, each img tag should have empty alt="" as you did and add aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images in( icon-view, icon-ethereum, icon-clock ).

  • Never use <div> and <span> alone to wrap a meaningful content. Just keep in mind that you should usually use semantic HTML in place of the div tag unless none of them (the semantic tags) really match the content to group together. By adding semantic tags to your document, you provide additional information about the document, which aids in communication.

  • You can use unordered list<ul> to wrap the class="spans". In each <li> should be <img> and <p> instead of <span> for the reason stated before, then you may use flex properties to align them centrally.

  • The avatar's alt should not be some-boy-avatar, it’s meaningless . You can use the creator's name Jules Wyvern. Read more how to write an alt text

  • To use more semantic tags , you may use <figure> and <figcaption> for the avatar's part.

  • If you wish to draw a horizontal line, you should do so using appropriate CSS. Remove the <hr>, you can use border-top: to the avatar's part.

  • There are so many ways to add the hover effect on the image , The one I would use, using pseudo-elements to change the teal bg color to a hsla. Then opacity can be changed from 0 to 1 on the pseudo element on hover. The icon view doesn’t really need to be in the HTML as there no need for another clutter in HTML. You can use CSS for it.

  Overall, your solution is great. Hopefully this feedback helps.

  1
• 

  POTB•330

  @PeteonthebeatPosted over 2 years ago

  Hello, @PhoenixDev22 Thank you for this thorough review of my submission. I really appreciate the constructive criticism. Hands down, this is the most extensive feedback I ever got on anything I did with code.

  I searched the provided instructions and readme file, but I never found a requirement for a hover state on the image or any of the text, but maybe I'm wrong, so I wrapped both 'Jules Wyvern' and 'Equilibrium #3429' into <a> tags. Now they have the hover effect going.

  I also applied your advice about using <ul> and <li> instead of plain <div> and that <span> which I learned recently and apparently decide to use on whatever came to my mind.

  I deleted the <hr> tag, adding border: 0.1rem solid white; to the creation <div>. This was very helpful as it reminded me how to add opacity to the property —  border-top: 0.2px solid rgba(255, 255, 255, 0.1)

  Other than that, I'm not sure wrapping my images inside <figure> or <figcaption> tags would actually improve things... Maybe it will but I left those as they are.

  Nonetheless, I updated the files, making corrections based on your feedback.

  Regards, POTB

  0