3 column preview card component

Community feedback

• 

  Hamzat Lawal•560

  @EngineerHamzieyPosted over 2 years ago

  You have done a great job 👏👏 Now that I've checked you site 😘 and your code here are my suggestions:

  • always apply background color on body element (NOT THE HTML).

  • Always try to organize your style-sheet to make it easy to work with. I can see you styling your footer immediately after the main. Try putting them in orderly manner(You might find yourself on a team tomorrow).

  • And don't be confused by the statement "The designs were created to the following widths: Mobile: 375px and Desktop: 1440px" what it means is that the design files(pictures in design folder) given has width of 375px for mobile and 1440px for desktop.

  • To explain further, this is what I always do to make sure my design is almost same size as the design given(i.e almost pixel perfect): to ensure that what I built as almost pixel perfect as possible: (since the design we are given for PC is of width 1440px according to the style guild) I always start by setting my browser screen-size to 1440px using developer tool or: method 2: - Since my PC screen is smaller than 1440px I'll temporarily set the width of my body to 1440px(width: 1440px) then I'll open the browser and zoom out until my the scroll-bar disappears, then I'll remove the width(width: 1440px) from the body. I don't know if this is explanatory enough

  • Your media query should be the normal one as shown below

  /* not min-width: 1440px, obviously there are several PCs out there whose screen is smaller than 1440px*/
  @media (min-width: 769px) {
  	main {
  		display: flex;
  	}
  }
  

  • concerning the HTML issue <main> <section class=car-cards></section> it's not needed in your code at all.the <section class=car-cards></section> is not needed in this code because it's empty, you don't use it.

  • use max-width on the card instead of width, and stick to em or rem unit for responsiveness. Giving the exact width like you did will compromise the responsiveness. eg, scroll-bar will appear on a smaller screen-size of a tab or mini-PC, while max-width will make it shrink to the achieve responsiveness. and it will also limit the width on wider(ultra-wide) screens.

  • you can store the colors in a variable to make work easier, and it will make you change the color easily(if needed) without having to visit all it occurrence in the code.

  /* Storing some properties like colour in a variable is a good idea and makes
  your work fast, you can easily type in the variable name without trying to remember
  the rgb or hex value of the color. It also make it easy to change values of all occurrence of the property.
  the `:root{}` is the best place to declare the variable to make it global*/
  :root {
     /*
      the syntax for variable declaration is simple, just start with double dash "--" as 
      shown below, 
      since the name is more than a word, I separated them with a dash, you can use camel case as well.
    */
    /* main background */
    --Very-dark-blue: hsl(233, 47%, 7%);
    /*  card background */
    --Dark-desaturated-blue: hsl(244, 38%, 16%);
    /* accent */
    --Soft-violet: hsl(277, 64%, 61%);
    /* main heading and stats */
    --White: hsl(0, 0%, 100%);
    --main-paragraph: hsla(0, 0%, 100%, 0.75);
    --Stats-headings: hsla(0, 0%, 100%, 0.6);
    --overlay: rgba(128, 10, 197, 0.5);
  }
  

  • You don't need to put height auto on the cards, I think that's the default already, because naturally the height of you content determines the height of the container.

  • I love the fact that your code is very neat, but I think it will be better if you can leave a line between each selector. like:

  p span {
  	text-transform: uppercase;
  }
  
  .sedans {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(31, 77%, 52%);
  }
  
  .suvs {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(184, 100%, 22%);
  }
  

  As opposed to:

  p span {
  	text-transform: uppercase;
  }
  .sedans {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(31, 77%, 52%);
  }
  .suvs {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(184, 100%, 22%);
  }
  

  • Try using rem / em for the font-size too. 15px = (15px / 16px)rem = 0.9375rem so you can use font-size: 0.9375rem;

  • Pack the guys😂 (selectors I mean) that have the common styling together: According to your code:

  .sedans {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(31, 77%, 52%);
  }
  .suvs {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(184, 100%, 22%);
  }
  .luxury {
  	width: 300px;
  	height: auto;
  	padding: 2em;
  	background-color: hsl(179, 100%, 13%);
  }
  

  You can make it shorter and neater:

  /* And don't forget the line between each selector*/
  .sedans, .suvs, .luxury {
    width: 300px;
    height: auto;
    padding: 2em;
  }
  
  .sedans {
  	background-color: hsl(31, 77%, 52%);
  }
  
  .suvs {
  	background-color: hsl(184, 100%, 22%);
  }
  
  .luxury {
  	background-color: hsl(179, 100%, 13%);
  }
  

  • that is all I noticed, I have done some some correction and explanation to someone's code you can check it,I think it will help...

  github repository link

  live site link

  if you notice anything wrong, feel free to correct me I am not perfect, I am a beginner, I am open to correction and suggestions and I am wiling to learn more I'm doing this because I hope it will help feel free to ask questions,

  I hope this was helpful and my hope my bad grammar isn't giving you a hard time 😊.

  2

  ---

  

  Paweł•1,030

  @Pawel-GnatPosted over 2 years ago

  @EngineerHamziey Don't worry about your English. I understand everything you have written here :) and it's not my primary language.

  Firstly, I have to say that your work is priceless! I noted everything and I will implement your advices into my further challenges. I know some of them, but it's hard to remember and code everything, especially as a rookie.

  Good pointing me on that pixel width task. I thought I have to use 1440 px :)

  I will stick to DRY method and learn how to properly use min-max width ;)

  Thanks and hope to see you again! :))

  1

  

  Hamzat Lawal•560

  @EngineerHamzieyPosted over 2 years ago

  @Pawel-Gnat you're always welcome. Even I myself submitted a solution just now, and I have seen atleast two mistakes after submitting. like you said,It's hard to remember even when you know it, Feel free to check it out and give me feedback 😊. Solution link

  0
• 

  Ron Paolo Toyhacao•165

  @rontoyhacaoPosted over 2 years ago

  Hello Paweł!

  Nice to hear that you've had the challenge easy and you did great! Upon seeing your work I see that your main section is positioned at the top and not at the center. Since you already have your body tag at flex, one last thingg you can add is height: 100vh to have it centered. That's all and happy codingg.

  2

  ---

  

  Paweł•1,030

  @Pawel-GnatPosted over 2 years ago

  @rontoyhacao Oh right, I forgot about centering my main. Thanks for your help :) Im happy that you find my code quite easy to read.

  1

  

  Hamzat Lawal•560

  @EngineerHamzieyPosted over 2 years ago

  @rontoyhacao using height will make the remaining of the page disappear on smaller screen instead of showing scroll bar, I mean screen with smaller height that can't contain the webpage Use min-height: 100vh; instead. Using min-height will make it expand/grow when needed

  You can experiment this using Google chrome developer tool.

  2

  

  Ron Paolo Toyhacao•165

  @rontoyhacaoPosted over 2 years ago

  @EngineerHamziey thankyouuusomuch for the tip I didn't know that, I'll take note of that.

  1

  

  Hamzat Lawal•560

  @EngineerHamzieyPosted over 2 years ago

  @rontoyhacao you're always welcome ma'am I'm sorry my other comment is incomplete. I'll edit it soon

  0