Four Card Responsive using 3 grids

Community feedback

• 

  Djamel1133•880

  @Djamel1133Posted about 1 month ago

  ok ...Keep it up and see you next challenge.

  Marked as helpful

  0
• 

  P

  Kamran Kiani•1,280

  @kaamiikPosted about 1 month ago

  • I see your heading like this: A h2 tag that has a strong tag inside of it. I think the whole is a heading.

  • Maybe It's better that your cards heading be a h3. It seems a one level reduce with your top page heading.

  • I suggest you add a proper CSS reset to your style. Andy Bell and Josh Comeau both have a good one.

  • Use rem for your font-size and max-width instead of px.

  • Your grid layout need 4 rows. It's better for your layout because If your content changes, your grid items will mess up. Check this link on discord: https://discord.com/channels/824970620529279006/1214884151728152577/1215761500225011872

  • Instead of height: 100vh; It's better to use min-height: 100vh. This works better.

  0

  ---

  

  P

  justine1607•110

  @justine1607Posted about 1 month ago

  @kaamiik thank you for your feedback.

  1. i will try to change the h2 into h3.
  2. yes next time will do add a proper css reset for my styles.
  3. i believe i use rem for my font-size, just in max-width i didn't but will change it to rem.
  4. for this I commented also https://discord.com/channels/824970620529279006/1214884151728152577/1215761500225011872
  5. okay noted on using min instead of height

  1
• 

  Djamel1133•880

  @Djamel1133Posted about 1 month ago

  Great job, and what clean code! I really liked it.

  I completed the same challenge on Frontend Mentor, and they asked me to provide feedback to other coders on it. So, I have a few questions if you don't mind:

  1. Why didn't you use variables for font-size and font-weight as you did for colors?

  :root {
    /* Primary colors */
    --Red: hsl(0, 78%, 62%);
    ...
    /* Font weights */
    --font-weight-small: 200;
    ...
    /* Font sizes */
    --font-size-small: 1rem;
    ...
    /* Spacing */
    --spacing-small: 1rem;
    ...
  }
  

  2. Why did you use px instead of rem?

    margin-bottom: 76px; 
    margin-top: 15px; 
    gap: 15px;
    ...
  

  You could also use calc or clamp for more flexibility. Check out this article: Typography in Web Design.

  3. Why did you use a div inside another div in your HTML code? Why not insert HTML elements within a single div like this:

  <div class="card__supervisor">
    <h3>Supervisor</h3>
    <p>Monitors activity to identify project roadblocks</p>
    <img src="images/icon-supervisor.svg" alt="Magnifying glass icon with an eye in the center">
  </div>
  

  Finally, I picked up some tricks from your code to optimize mine. Thank you, and well done again!

  0

  ---

  

  P

  justine1607•110

  @justine1607Posted about 1 month ago

  @Djamel1133 thank you for your'e feedback I do appreciate it. for your questions:

  1. I believe that the font-size and the font-weight should really be there, it just i forgot to put that there.
  2. i use px for this because i want to not change so it will have that value as always.
  3. i use another div inside the div, it's purpose is for wrapping the element so later we can add style on it.

  0