Stats preview card component with SCSS

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted about 3 years ago

  👋Hi There!

  👍 Good job! Now, your files are clean from development code.

  Some feedback:

  • For more semantic HTML, I recommend to swap the stats div with ul and wrap each item with li instead of div.
  • The stats number should not be heading tags. When you want to use heading tags, think of it like a putting title on document file. I recommend to use paragraph elements instead.
  • There are two header divs, I recommend to only use one of them and changed the image using background-image on @media query or try to use picture tag instead.
  • I recommend to keep the specificity as low as possible by selecting the element directly (without keep adding the parent class name). In this case, in my opinion it's possible to style any elements without any complex selectors.

  /* ❌ Adding parent element */
  .card .content__title {
    /* some styles */
  }
  
  /* ✔️ Directly select the element */
  .content__title {
    /* some styles */
  }
  

  • Use rem or sometimes em unit, instead of px. Using px won't allow the users to control the size of the elements based on their needs.
  • For letter spacing, it's recommended to use em unit, so when the text get bigger the letter spacing will automatically get bigger too.

  That's it! Hopefully this is helpful!

  0

  ---

  

  Ctrl+FJ•810

  @FlorianJourdePosted about 3 years ago

  @vanzasetia Thank you again for this very detailled feedback. That's crazy, everything you say seems so logical once you reed it.

  I'll, for sure, be more attentive to everything on next challenge ! Talk you later, hoppefully

  0

  

  Ctrl+FJ•810

  @FlorianJourdePosted about 3 years ago

  @vanzasetia Ok, then ! If you want to continue to make feedback to my work, here's my solution for the next challenge !

  I tried to follow as much advises you pointed. Just wanted to let you know that complex selectors seems to come from SCSS syntax. This kind of :

  .card .content__title {
    /* some styles */
  }
  

  When my code is not compiled, it generaly appears with only one class !

  Thanks for interest, again !

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted about 3 years ago

  @FlorianJourde That CSS or compiled code is coming from this Sass code:

  .card {
    text-align: center;
    background: $dark-desaturated-blue;
    max-width: 330px;
    margin: auto;
    border-radius: 10px;
    display: flex;
    flex-direction: column;
  
    .content {
      padding: 35px;
  
      &__title {
        margin-bottom: 15px;
      }
  

  The card element is the parent element and then you use the & as the parent selector, which is referring to .content. So that's why, it gets compiled to:

  .card .content__title {
    /* some styles */
  }
  

  What I recommend is that you don't need to nest the content inside the .card element.

  .content {
      padding: 35px;
  
    &__title {
      margin-bottom: 15px;
    }
  }
  

  So, it get compiled to this.

  .content__title {
    /* some styles */
  }
  

  0

  

  Ctrl+FJ•810

  @FlorianJourdePosted about 3 years ago

  @vanzasetia Okay, I've got it. Nesting with SCSS is not always needed, I'll try to be careful, thanks !

  0
• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @FlorianJourde 👋🏻

  You've got it look almost identical to the original design, however, there's one small thing I'd suggest changing, and that's min-width of the media query. I think it should be around 700px, because the image gets under the heading's container and stays like that until it reaches mobile viewport width.

  I hope this was helpful 👨🏻‍💻 Cheers 👾

  0

  ---

  

  Ctrl+FJ•810

  @FlorianJourdePosted about 3 years ago

  @kens-visuals I struggled a while with width (and overlay, as well), so it is helpful !

  Thank you !

  0