3-column-preview-card-component-main

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hi there 👋. Good job on completing the challenge ! I have some feedback for you if you want to improve your code.

  I don't get space at the end of the page in mobile view. can anyone please help? You don't have space below because it never gets set and space e above is because your component overflows

  Currently I consider that he has written some unnecessary lines of code :( .:

  html, body {
      /* height: 100vh; */
  }
  body {
     /* To center it you can place flex-box directly on the body */
      display: flex;
      justify-content: center;
      align-items: center;
      min-height: 100vh;
  }
  
  .maincontainer {
      /* display: flex; */
      /* justify-content: center; */
      /* align-items: center; */
      /* height: 100vh; */
  }
  
  .container {
      max-width: 816px; /* Set a maximum width to the container, not to each individual card */
      margin: 20px; /* Add a margin */
      display: flex;
      border-radius: 10px;
      overflow: hidden;
  }
  
  .component {
      /* width: 17rem; */ /* Set a maximum width to the container */
      display: flex;
      flex-direction: column;
      padding-left: 2rem;
      padding-top: 20px;
  }
  
  @media (max-width: 550px)
  html, body {
      /* height: 100%; */
      /* height: 150vh; */
      /* padding: 1rem 0rem; */
      /* padding-top: 0rem; */
  }
  
  .container {
      /* All this is unnecessary */
      /* width: 100vw; */
      /* display: flex; */
      flex-direction: column;
      /* height: 90vh; */
      /* overflow: visible; */
  }
  @media (max-width: 550px)
  .sedan {
      /* This is unnecessary, it already has the border */
      /* border-top-left-radius: 0.5rem; */
      /* border-top-right-radius: 0.5rem; */
  }
  
  .luxury {
      /* This is unnecessary, it already has the border */
      /* border-bottom-left-radius: 0.5rem; */
      /* border-bottom-right-radius: 0.5rem; */
  }
  
  .component {
      /* Instead of setting a width of 80vw, let it take up 100% of the body and just add space using a margin */
      /* width: 80vw; */
      /* margin: auto; */
  }
  

  HTML:

  • Use the <main> tag to wrap all the main content of the page instead of the <div> tag. With this semantic element you can improve the accessibility of your page.

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

  • You should use the <a> tag instead of the <button> tag because the Learn More button is a link to another page. Use buttons to perform actions like submitting a form or closing a modal and use links to navigate to another page. You can read more about this here.

  • Not all images should have alt text. Car icons are for decoration purposes only, so they can be hidden from screen-readers by adding aria-hidden="true" and leaving its alt attribute empty:

  <img src="./images/icon-sedans.svg" alt aria-hidden="true">
  <img src="./images/icon-suvs.svg" alt aria-hidden="true" >
  <img src="./images/icon-luxury.svg" alt aria-hidden="true" >
  

  • For specificity reasons you should work with classes instead of ids because they are more reusable. You can use ids to work with JavaScript, but you should use classes to style your elements. You can read more about this here.

  I hope you find it useful! 😄 Above all, the solution you submitted is great!

  Happy coding!

  Marked as helpful

  1
• 

  Hassia Issah•50,650

  @HassiaiPosted almost 2 years ago

  There is no need to give the body and . container a height value. To center .container on the page using flexbox, replace the height of .main-container with a min-height of 100vh. min-height: 100vh;

  Give .component a padding value for all the sides, instead of give the img a padding-top value and the button a margin-bottom value. e.g: .component{ padding: 5rem;} this padding is a responsive replacement of the height value in .container.

  There is no need to restyle .maincontainer in the media query, again remove the height value and give .container a margin top and bottom value. For a responsive content, in the media query, replace the width of .container with a max-width.

  Use rem or em as unit for the padding, margin and width values. for more on CSS units Click here

  Hope am helpful.

  Well done for completing this challenge. HAPPY CODING

  Marked as helpful

  1
• 

  Vishanthan shree•130

  @vishanthan8055Posted almost 2 years ago

  Hey deeviesh!!!

  Welcome to frontendmentor!!!

  Your solution is amazing. But you need to follow one thing. You have to read the style guide carefully. I noticed that you missed the body background colour.

  Implement a Mobile First approach 📱 > 🖥 With mobile devices being the predominant way that people view websites/content. It is more crucial than ever to ensure that your website/content looks presentable on all mobile devices. To achieve this, you start building your website/content for smaller screen first and then adjust your content for larger screens.

  Happy coding!!

  Marked as helpful

  1