NFT-preview-card

Community feedback

• 

  Naveen Gumaste•10,480

  @NaveenGumastePosted almost 3 years ago

  Hay ! sushil 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

  Keep up the good work!

  Marked as helpful

  0

  ---

  

  sushil540•40

  @sushil540Posted almost 3 years ago

  @Crazimonk Thank you for your valuable feedback.

  1
• 

  Rimka•600

  @karimfilaliPosted almost 3 years ago

  Hello, congratulations for finishing your second FrontEndMentor challenge ! Your card is very great. The card is a little small but the sizes fit good with the expected design. However, you have an accessibility issue. To fix it, your website must have a h1 for accessibility reasons. Your p.t1 should be a h1. But use <h1> tag only once in main heading element.

  Very great job though !

  Marked as helpful

  0

  ---

  

  sushil540•40

  @sushil540Posted almost 3 years ago

  @karimfilali Thank you for your valuable feedback!, it's really worked for me.😊

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted almost 3 years ago

  Hello @sushil540 ,

  I have some suggestions regarding your solution:

  • you're using widths in percentage. Not a great idea as you're losing control of the layout. Use max-width instead, let it grow to a point. Then a little margin on the component or padding on the body to stop it hitting screen edges .let the content to dictate its height .

  .card {
     :
      max-width: 350px; 
      /* height: 460px; */
      display: block;
      :
  /*    margin: 5% auto 5% auto;*/   /* no need for margin if you are going to use flexbox*/
  :
  }
  

  • To center the card on the middle of the page, use the flexbox properties and min-height to the body.

  body {
     background-color: hsl(217, 54%, 11%);
     font-family: 'Outfit',sans-serif;
     display: flex;
     align-items: center;
     justify-content: center;
     min-height: 100vh;
  

  • <svg> 's do not add important information to a document should be considered decorative. You can use aria-hidden="true" to hide the SVG from screen readers. focusable="false"is also used to ensure Internet Explorer won’t allow the Tab key to navigate into the SVG.

  • for the image hover, first it should be wrapped in an anchor tag as already stated. You can add - either aria label or Sr only text or alt text on the image that says where the link goes.

  • The eye image 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 as it's a decorative image.

  • You can use an unordered list <ul> to wrap class="table"and in each list item would be <svg> and <p>. (rather than a table)

   <main class="card">
     <div class=images>
       <img class="svg" src="images/icon-view.svg" alt="a graphics">
         <img class="eq" src="images/image-equilibrium.jpg" alt="Equilibrium">
     </div>
         <h3> Equilibrium #3429 </h3>/* you need to use heading for the title , in this challenge , you can use <h1> */
     <h1 class="t1"> Our Equilibrium collection promotes balance and calm. </h1> /* use <p> tag */
  <table class="table"> /* You can use <ul> */
  :
  :
  :
    <div class="">
      <img class="pro" src="images/image-avatar.png" alt="avatar">
      <p class="t2"> Creation of <a>Jules Wyvern</a> </p>
    </div>
   </main>
  
  

  • No need for the <hr>, you can use border-top: ; to the avatar's div.

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

  • You can have a look at my solution to see the image part

  Hopefully this feedback helps.

  0

  ---

  

  sushil540•40

  @sushil540Posted almost 3 years ago

  @PhoenixDev22 Thanks for your excellent feedback.🙂

  1