Four Card Responsive using 3 grids

Community feedback

• 

  Djamel1133•910

  @Djamel1133

  Posted 9 months ago

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

  00

  Marked as helpful
• 

  P

  Kamran Kiani•2,780

  @kaamiik

  Posted 9 months 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.

  01
• 

  Djamel1133•910

  @Djamel1133

  Posted 9 months 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!

  01