NFT Preview Card | Frontend Mentor Challange

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hello @Apprentixx ,

  Congratulation on completing 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 , 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 the hover as there is no reason to have the extra clutter in the html.

  • The avatar's alt should not be "avatar-img". It's meaningless , 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.

  • No need for <div class="line1"></div>, you can use border-top: to the class="bottom".

  • To use more semantic tags , you can use <ul> to wrap class="ETH"and in each <li> there would be<img>and <p>.

  • For the avatar's part class="card-footer" , you would use <figure> and <figcaption>.

  • using min-height: 100vh instead of height: 100vh;allows the body to to grow taller if the content outgrows the visible page.

  • width: 350px;an explicit width is not a good way . consider using max-width to card instead and a little margin to the card .That will let it shrink a little when it needs to.

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

  • In line-height: 28px;use unitless value for the line-height , this is the preferred way to set line-height and avoid unexpected results due to inheritance.Read more in MDN.

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

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

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

  0
• 

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

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

  Also I have some tips for improving your code:

  • add main tag and wrap the card for Accessibility
  • remove all margin from card class because with flex they are superfluous
  • add justify-content: center; to body for center the card horizontally
  • instead of using px try to use relative units of measurement -> read here
  • You can add the effect :hover creating a div that appears on hover. I used tailwind but you can still see and understand which css properties you can use to do the same. Look here -> my solution
  • after, try to add a little transition on the element with hover effect

  Overall you did well :)

  Hope this help and happy coding!

  0