Nft-preview card component

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  Hello Emmanuel Munyite,

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

  • You can use <footer> landmark the attribution. HTML5 landmark elements are used to improve navigation.

  • The most important part in this challenge is the interactive element. 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>.

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

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

  • 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-ethereum, icon-clock).

  • You can use unordered list<ul> to wrap class="price". In each <li> should be <img> and <p>.

  • If you wish to draw a horizontal line, you should do so using appropriate CSS. You may remove the <div class="line"></div>, you can use border-top: to the avatar's part.

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

  • 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. There is no need for a extra clutter in the HTML. The icon view does not really need to be in the HTML . You can use CSS for it.

  • 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.

  Aside these, great job on this one. Hopefully this feedback helps.

  Marked as helpful

  0
• 

  Lucas 👾•104,340

  @correlucasPosted about 2 years ago

  👾Hello Emmanuel, congratulations for your new solution!

  Here's some tips to improve your solution:

  You need to place the background-color inside the body not the container to have it displaying all the colors. To make it align the component vertically first of all add to the body min-height: 100vh to make the body display 100% of the browser screen size and display: flex e flex-direction: column to align the child element vertically using the body as reference.

  body {
      background-color: var(--very-dark-blue-main-bg);
      min-height: 100vh;
      /* width: 100%; */
      /* height: 100%; */
      font-size: var(--font-size);
      font-family: var(--ff-primary);
      display: flex;
      align-items: center;
      justify-content: center;
  }
  
  main {
      /* width: 100%; */
      /* height: 100%; */
      background-color: var(--very-dark-blue-main-bg);
      display: flex;
      flex-direction: column;
      align-items: center;
  }
  

  👋 I hope this helps you and happy coding!

  Marked as helpful

  0
• 

  Emmanuel Munyite•510

  @munyite001Posted about 2 years ago

  Thanks, reply appreciate the tips, will start working on them immediately

  0