Four card feature section master with CSS Grid en SASS

Community feedback

• 

  Gregg Christofferson•295

  @gchristoffersonPosted about 3 years ago

  Hey @guztrillo! ✋

  Excellent job! Looks really good!

  I had a similar approach to the border-top as @markup-mitchell but instead of adding another level to the card markup, I opted for using the ::before pseudo-element. Thanks to @markup-mitchell codepen example, I figured out what was wrong with mine. I needed to add overflow: hidden to the card. Here's my example ->

  Marked as helpful

  0

  ---

  

  Carlos Guzman•285

  @guztrilloPosted about 3 years ago

  @gchristofferson Mark kill it with the demo! Your approach is very good too and save some extra HTML. Thanks for your review and solution too.

  0
• 

  Mark Mitchell•1,820

  @markup-mitchellPosted about 3 years ago

  Oh, and I noticed that between 576px and 847px viewport width the layout "breaks" in a way I assume is unintended for this (broadly speaking) tablet range.

  It reflects well on your implementation choices that the layout still looks good and could conceivably be intentional!

  Marked as helpful

  0

  ---

  

  Carlos Guzman•285

  @guztrilloPosted about 3 years ago

  @markup-mitchell Hey Mark. Wow, I really appreciate you took your time to review my code. Some things I want to point out:

  1. Thanks for the demo, I had no idea. I assume most of the magic occurs with the overflow: hidden. I will fix mine now.

  2. I wasn't considering the order of the imports, and you're completely right. I never thought about this before but I will fix it and follow this approach from now on.

  3. Yes, I probably should nested more items and use more variables. I'm not an SASS expert neither, as you can see haha. But I started to use to practice with it and complete some challenges. Actually, I thought about this when I submit it. So yes, I'm gonna modify this too.

  4. About the layout "breaking" at that viewport, I was aware about it but I thought it looked different and somehow cool and wanted to keep it that way. In fact, I didn't want to change it because didn't want to add extra media queries. All the design was conceived with the original grid-template-columns property. But thanks to notice this.

  0
• 

  Mark Mitchell•1,820

  @markup-mitchellPosted about 3 years ago

  Hi Carlos,

  There's no single "right" solution and yours looks really good to me! The use of container elements looks very economical and the semantic elements and headings hierarchy looks good. You really went all-out with grid didn't you?! My instinct would have been to use flexbox more for the grid-items, but I'm not sure whether that would offer any advantages, really.

  For the top border I'd add another level to the card markup. Put the border on the inner element and the shadow and border radius on the outer element. demo.

  You might want to have a look at the order of your sass imports. By importing base third you end up with

  * {
    box-sizing: border-box;
    padding: 0;
    margin: 0; }
  

  being declared some way down your outputted stylesheet... it doesn't seem to be causing a problem here, but it could easily do in a larger project. Your base styles probably want to be imported first and then overridden as your specificity increases.

  I'm not super familiar with sass, but I feel like you're not taking full advantage of its features? You could be nesting selectors like:

  .cards-header {
       display: grid;
       gap: 1rem;
       place-content: center;
       place-items: center;
  
       h1 {
            font-weight: 200;
            text-align: center;
            max-width: 30rem;
       }
  
       p {
            max-width: 32rem;
            text-align: center;
       }
  }
  

  and using sass (or just css custom variables) to store common values. For example you have font-size: 15px in quite a few places, and for reasons of responsiveness and scaling that might better be expressed as a rem value.

  If you declared

  :root {
      --font-size--small: 15px;
  }
  

  and set all the instances of font-size to font-size: var(--font-size--small) you could update the variable definition like so:

  :root {
      --font-size--small: 2rem;
  }
  

  and it would update everywhere at once.

  I had to look really hard at your project to find these small suggestions... I hope I've made sense!

  Marked as helpful

  0
• 

  Mark Mitchell•1,820

  @markup-mitchellPosted about 3 years ago

  My pleasure Carlos! I think the "tablet" layout looks cool too, but good luck convincing a UX designer 😂

  0