3-column-preview-card-component-main

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted 11 months ago

  Hello there 👋. Good job on completing the challenge !

  I have other suggestions about your code that might interest you.

  • You should use only one <h1> tag per page. The <h1> tag is the most important heading tag, This can confuse screen reader users and search engines. This challenge requires that Sedans, SUVs and Luxury are headings, but you can use the <h2> tag instead of the <h1> tag. You can read more about this here 📘.

  • The "<div class="learmore">" should be a button and not a div. Since the element has a hover effect and is interactive in the design, it's better to use a interactive element like links or buttons for accessibility. Also remember to add a cursor: pointer

  I hope you find it useful! 😄

  Happy coding!

  1
• 

  Marcos Travaglini•4,840

  @BlackpachamamePosted 11 months ago

  Greetings @arowstev! There are a few things to fix:

  • The images are not visible, this is because you placed the paths wrong, from src="/images/icon-suvs.svg" they should be `src="./icon-suvs.svg"
  • The header is empty, it has no use, it is better to delete it
  • The <div class="mainbox"> should be <main class="mainbox">, the previous main just delete it, it serves no purpose
  • The <div class="attribution"> should be <footer class="attribution">, please leave it out of the main
  • To center the content do not use position: absolute, rather use flex:

  body {
      min-height: 100vh;
      display: flex;
      justify-content: center;
      align-items: center;
      flex-direction: column;
      gap: 20px;
  }
  
  .mainbox {
      display: flex;
      min-height: 400px; /* This change adapts better to the screen change */
      max-width: 800px; /* This change adapts better to the screen change */
      /* position: absolute; */
      /* top: 50%; */
      /* left: 52%; */
      /* transform: translate(-50%, -50%); */
      border-radius: 5px 5px 5px 5px;
  }
  
  @media (max-width: 1250px){
    .mainbox {
      flex-direction: column;
      min-height: 400px; /* This change adapts better to the screen change */
      max-width: 800px; /* This change adapts better to the screen change */
    }
  
   .mainboxone {
      border-radius: 5px 5px 0 0;
      padding-bottom: 20px;
   }
  
   .mainboxtwo {
      padding-bottom: 20px;
   }
  
   .mainboxthree {
      border-radius: 0 0 5px 5px;
      padding-bottom: 20px;
   }
  }
  

  1
• 

  arowstev•120

  @arowstevPosted 11 months ago

  Thank you for the feedback. I will note all the corrections.

  0
• 

  Daniel 🛸•44,310

  @danielmrz-devPosted 11 months ago

  Hello @arowstev!

  You did a very good job there!

  I have just a couple of very simple suggestions for improvement:

  • First: Since the buttons Learn more are clickable element, it's nice to add cursor: pointer to them.

  • Second: Your icons are not showing because you set the incorrect path for them.

  This is your code:

  <img class="sedaniconimage" src="/images/icon-suvs.svg" alt="">
  

  But since they are in the same folder than the HTML file, you have to remove /images/ from the path, like this:

  <img class="sedaniconimage" src="icon-suvs.svg" alt="">
  

  Advice: Keep the assets of the project (images, fonts) in a separate folder. It helps with the organization of the project.

  I hope it helps!

  Other than those details, you did a great job!

  0