NFT preview card component

Community feedback

• 

  PhoenixDev22•16,830

  @PhoenixDev22

  Posted over 2 years ago

  Hi Welangai Eric,

  Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:

  HTML

  • Page should contain <h1> . The <h1> is most commonly used to mark up a web page title. This challenge is supposed to be one component of a web page. To tackle the accessibility issue in the report , you may use an <h1> visually hidden with class=”sr-only”. You can find it here.

  • The most important part in this challenge interactive elements. 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 you can use <a>.

  You should have used <a> to wrap Equilibrium #3429 and Jules Wyvern too.

  • The link wrapping the equilibrium image should either have Sr-only text, an aria-label or alt text that says where that link takes you.

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

  • Profile images like that avatar are valuable content. The alternate text should not be avatar.You can use the creator's name Jules Wyvern. Read more how to write an alt text .

  • look up a bit more about how and when to write alt text on images. Learn the differences with decorative/meaningless images vs important content

  • You should use <p> instead of <h4> in <h4> 0.041 ETH</h4>.

  • There are so many ways to do the hover effect on the image, The one I would use is pseudo elements::before, ::after. You can use pseudo-elements to change the teal background color to hsla. Then the opacity can be changed from 0 to 1 on the pseudo element on the hover. Also using pseudo elements makes your HTML more cleaner as there's no need for extra clutter in the HTML.

  • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.

  CSS

  • Consider using min-height: 100vh instead of height: 100vh to the body , that let the body grows taller if the content of the page outgrows the visible page.

  • width:350px; an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.

  • height: 600px; It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.

  • The icon view does not really need to be in the HTML. You can use CSS for it.

  • Remember a modern css reset on every project that make all browsers display elements the same. Set the image display: block ; as there is a little gap under the image, and that's way you have used height: 98%.

  • Last, Don’t Repeat Your CSS is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.

  Hopefully this feedback helps.

  00

  Marked as helpful