NFT preview card component

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hello @prettyBcoding,

  Congratulation on completing another frontend mentor challenge.

  I have some suggestions regarding your solution:

  • Anything with a hover style in design means it's interactive. you need to add an interactive element<a> around the image , Equilibrium #3429 and Jules Wyvern.

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

  • the icon-view doesn't really need to be in the html, you could do it with css. If you want it to stay in html it needs to be aria-hidden or role presentation with empty alt.

  • There are so many ways to add the hover effect on the image , The one I would use 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 as there is no reason to have the extra clutter in the html.

  • The avatar's alt should not be "creator's photo ", you can use Jules Wyvern. Read more how to write an alt text

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

  • You can use <ul> to wrap it and in each <li> there would be <img> and <p> .

  <ul class="">
      <li>
          <img class="icon-ethereum" src="images/icon-ethereum.svg" alt="" aria-hidden="true">
          <p class="price">0.041 ETH</p>
      </li>
       <li>
            <img class="icon-clock" src="images/icon-clock.svg" alt="" aria-hidden="true">
          <p class="time">3 days left</p>
        </li>
  </ul>
         
  

  • No need for <hr> , you can use border-top: to the avatar part.

  • Fo the avatar's part , you would use <figure> and <figcaption>.

  • Use a little css reset to remove the default margin from the body

  *{
  margin:0;
  padding: 0;
  box-sizing: border-box;
  }
  

  • width: 20rem; an explicit width is not a good way . consider using max-width to card instead That will let it shrink a little when it needs to and a little margin to the card that will prevent it hitting the screen edges.

  • height: 35rem;It's rarely ever a good practice to set heights on elements . Let the content inside the card element dictate the height of it.

  • There's a little gap under the main image , give display: block that would fix this.

  • You should use em and rem units .Both em and rem are flexible, Using px won't allow the user to control the font size based on their needs.

  General point : Remember a css reset on every project. That will do things like set the images to display block and make all browsers display elements the same.

  You might have a look at my solution , to see how I did the hover effect on the image , it might my helps.

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

  0
• 

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  Hi Benolísio, I took some time to look at your solution and you did a great job!

  Also I have some tips for improving your code:

  • remove all margin from card class
  • To make it look as close to the design as possible remove opacity from .overlay class and use the rgba() for the backgound color instead the hexadecimal color
  • try to use flexbox to the body for center the card. Read here -> best flex guide
  • after, add min-heigth: 100vh to body because Flexbox aligns child items to the size of the parent container

  Overall you did well :)

  Hope this help and happy coding!

  0
• Account deleted

  Hi there,

  • remove the margin from the main and you can center the body by giving The body these properties to center the element display:flex justifiy-content:center align items: center min-height:100vh

  i hope this is helpful and goodluck

  0

  ---

  

  Benolísio•60

  @prettyBcodingPosted over 2 years ago

  @Old1337 Did it. But not changed

  0

  Account deleted

  @prettyBcoding are you sure you have removed the margin?

  0

  Account deleted

  @prettyBcoding are you sure you have removed the margin?

  0

  

  Benolísio•60

  @prettyBcodingPosted over 2 years ago

  @Old1337 yeah. You can see the code

  0

  Account deleted

  @prettyBcoding it got fixed as i can see

  0

  

  Shashree Samuel•9,260

  @shashreesamuelPosted over 2 years ago

  @prettyBcoding

  Make sure it looks like this

  justify-content: center;
  align-items: center;
  min-height: 100vh;
  

  I hope this helps

  Cheers, Happy coding 👍

  0