CSS and HTML

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  With grid, it is fine to use, but is designed for 2 dimensional layouts. As you are only laying things out in one direction - row / column, flexbox is actually simpler.

  I just had a play in devtools, which I recommend you doing to understand the different options available to you:

  .card {
    /* display: grid; */
    /* grid-template-columns: repeat(2, 500px); */
    display: flex;
    grid-template-columns: repeat(2, 1fr);
    max-width: 60rem;
  }
  
  .image__tint {
    /* opacity: 0.5; */
    opacity: 0.8;
    mix-blend-mode: multiply;
  }
  
  @media screen and (max-width: 800px) {
    .card {
      /* margin-top: 2rem; */
      /* margin-bottom: 2rem; */
      /* grid-template-columns: 300px; */
      /* grid-template-rows: repeat(2, max-content); */
      /* height: max-content; */
      flex-direction: column-reverse;
      max-width: 30rem;
    }
    .text {
      /* padding: 1rem; */
      padding: 2rem;
    }
    .card * {
      /* width: 100%; */
    }
    .image {
      /* height: 200px; */
      /* grid-column: 1/2; */
      /* grid-row: 1/2; */
      min-height: 200px;
    }
  }
  
  .main__paragraph {
    /* width: 90%; */
    /* line-height: 1.5rem; */ // no need for unit on line-height
    line-height: 1.7;
  }
  
  .image {
    /* grid-column: 2/3; */
    /* grid-row: 1/2; */
    /* height: 100%; */
    flex: 1 1 50%;
    background-position: center right;
  }
  
  .text {
    /* align-self: center; */
    /* grid-column: 1/2; */
    /* grid-row: 1/2; */
    /* padding: 4rem; */
    padding: 3rem;
    flex: 1 1 50%;
  }
  
  :root {
    /* --font-size-bc: 15px; */
    --font-size-bc: 0.9375rem;
  }
  
  main {
    padding: 2rem;
  }
  
  html {
    /* font-size: 15px; */
    font-size: 0.9375rem;
  }
  
  

  1

  ---

  

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  Note, I changed the media query to 800px - The reason is because there is room for the layout to switch at that point. You're not forcing small desktops to use a mobile layout

  0

  

  Nilanshu V Rajmane•125

  @nilanshu96Posted over 3 years ago

  @grace-snow Thanks a lot! This is really helpful. You gave a really quality feedback. It might seem less but I got to learn a lot from this.

  0
• 

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  Hi I think this still needs some work. In particular

  • try not to call multiple css files (poor for performance)
  • look up html semantics. It's really important to use meaningful html elements but you're not using any at all at the moment
  • never have font sizes in px, always a responsive unit like rem/em
  • let your media query start much earlier than what you have atm, as soon as there is room for the layout to switch from mobile to desktop, that's when a media query should kick in
  • try to vertically center your component like the design
  • fix the mobile alignment of the stats
  • remember details like line height
  • consider using flex not grid for the layout as it's only going in one dimension

  I hope these tips help

  1

  ---

  

  Nilanshu V Rajmane•125

  @nilanshu96Posted over 3 years ago

  Hi, thanks a lot for your valuable feedback! I would like to answer some of your points here

  • I'm aware of the performance issue. I normally use a webpacker to compress the css files into one but here I wanted to separate the card css from rest of the page so that people can easily go through it.
  • Thanks for pointing out the semantics. I completely forgot about them as a newbie.
  • I only used the font-size in px for html element because that's what the style guide required me to do
  • Why is it good to have media query earlier? Does it provide any benefit?
  • Thanks for pointing out the vertical centering. It didn't catch my eye at all.
  • I'll fix the mobile alignment. I made a major mistake here.
  • I tried using line height but I can't understand yet how to use it properly. I'll take a look into it.
  • I used grid because I thought it was easier to change the column and row position of elements within the card using grid. Does flex provide any advantage here?

  0
• 

  Dinesh•1,115

  @Dinesh1042Posted over 3 years ago

  Hello, NILANSHU RAJMANE'S!👋

  Well done on this challenge

  • you can use border-radius for both of your child element image,image__tint.
  • When resizing the screen the card hits the end. there is no breathing space for the card. you can add margin get some breathing space.

  Amazing work. Happy Coding💪

  1

  ---

  

  Nilanshu V Rajmane•125

  @nilanshu96Posted over 3 years ago

  Thanks a lot for your feedback!

  0