nft-preview-card-component

Community feedback

• 

  Md Raihan Alam•520

  @Md-Raihan-AlamPosted almost 3 years ago

  You forgot to add hover effects in here. Doing the [Ethereum/three days later] section vertically aligning will be much easier doing with flex/grid (whichever you prefer). Try to complete next project using grid/flex.

  1

  ---

  

  steezyza•90

  @steezyzaPosted almost 3 years ago

  @Md-Raihan-Alam I'm going to add the hover effects, make other layout changes, then resubbit.

  I will try grid to position my items. Flex results in my code not being clean. Thank you very much for the tip.

  0
• 

  Naveen Gumaste•10,480

  @NaveenGumastePosted almost 3 years ago

  Hay ! steezyza Good Job on challenge

  These below mentioned tricks will help you remove any Accessibility Issues

  -> Add Main tag after body <main class="container"></main>

  -> Learn more on accessibility issues

  If this comment helps you then pls mark it as helpful!

  Have a good day and keep coding 👍!

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted almost 3 years ago

  Hello @steezyza,

  I have some suggestions regarding your solution:

  • There should be two landmark components as children of the body element - a main (which will be the card all the elements should be in the same container ) and a footer (which will be the attribution). no need for <header>in this challenge.

  • Anything with a hover style in a design means it's interactive. you need to add an interactive element around the image Equilibrium #3429, Jules Wyvern . Add the hover effects on them as well.

  • for this one <h3>Equilibrium #3429</h3> , use a header. you can use an <h1> or <h2> and have <h1> with sr-only "` (Hidden visually, but present for assistive tech).

  • 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 ( alt="ethereum icon", alt="clock icon").

  • The alt text of the shouldn't be `avatar `, it should beJules Wyvern ``.Tips for writing 'good' alt text

  • 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:true or role presentation with empty alt

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

  • You can use an unordered list<ul> to wrap class="section-details"and in each <li>, there would be <img > and<p>.(no need for <h5>)

  • No need for <div class="horizontal-line"></div>, you can use border-top to the avatar's div .(in the html , there is <hr> does the same ).

  • For your question, To center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh like this : (no need for the absolute position )

  body{
  display:flex;
  align-items: center;
  justify-content: center;
  min-height: 100vh;
  

  • Never use px for font-size.

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

    • an explicit width is not a good way . Remove the width from the card component and change it to max width instead for the card. That will let it shrink a little when it needs to.

  • I also recommend to remove the height of the card , you almost never want to set it , let the content define the height.

  Overall , your solution is good . Hopefully this feedback helps

  0
• 

  Naveen Gumaste•10,480

  @NaveenGumastePosted almost 3 years ago

  Hay ! steezyza Good Job on challenge

  These below mentioned tricks will help you remove any Accessibility Issues

  -> Add Main tag after body <main class="container"></main>

  -> Learn more on accessibility issues

  If this comment helps you then pls mark it as helpful!

  Have a good day and keep coding 👍!

  0