SASS, FLEX, GRID, VARIABLES; MIXING

Community feedback

• 

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  Hi Axel, great work on this challenge! 😉

  Here are a few tips for improve your code:

  • Tip of graphic design: with font-family:" Big Shoulders Display ", cursive the browser will use the Comics Sans font when it doesn't find the first font indicated (you can seen during loading)... for the designer it's a really awful font! I would rather replace it with a font-family:" Big Shoulders Display ", sans-serif much more similar to the primary font.
  • add main tag and wrap the card for improve the Accessibility
  • add descriptive text in the alt attribute of the images
  • remove all unnecessary code, the less you write the better as well as being clearer: for example the html comments
  • to make it look as close to the design as possible add border-radius: .5rem and overflow: hidden to .grid__component class and remove height property
  • add min-height: 100vh and margin: 0 to body because Flexbox aligns child items to the size of the parent container
  • instead of using px use relative units of measurement like rem -> read here

  Overall you did well 😁 Hope this help!

  Marked as helpful

  0

  ---

  

  Axel•70

  @Axelh98Posted over 2 years ago

  @denielden Thank you for your feedback, I really appreciate it. Is "px" an old unit of measurement?

  1

  

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  @Axelh98 You are welcome! You could say yes but more than old it is not responsive. Happy coding ;)

  0
• 

  Prabhash Ranjan•2,540

  @besttlookkPosted over 2 years ago

  • As you are new to sass i would suggest using "@use/@forward" instead of "@import". In futrue @import will be deprecated.
  • Currently the content is not centered w.r.t the screen . Even tho you used flex to do that. You just missed one thing.

  body {
      display: flex;
      justify-content: center;
      align-items: center;
     min-height:100vh;   //this is very important
    }
  
  

  • Dont forget to reset the padding and margin for better control in your design. Try to add below code in every project you make. Add it at the top

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

  • If you want to write well maintened code. Do consider the order in which you add the property in css. Here is a link you can refer:

  https://9elements.com/css-rule-order/

  Good luck,

  Happy Coding

  Marked as helpful

  0

  ---

  

  Axel•70

  @Axelh98Posted over 2 years ago

  @besttlookk This is very helpful for me. Thank you for taking a minute and giving me feedback.

  I have a question about min-height: 100vh. may I use this code if I write "margin: 0 auto" instead of display flex ...center ...center ?

  I'm gonna learn the code order.

  Thank youu!

  0

  

  Prabhash Ranjan•2,540

  @besttlookkPosted over 2 years ago

  @Axelh98 margin: 0 auto can always be used if you want to center the element horizontally. But If you want to center the element in both direction ..use flex.

  One pro tip tho: In place of "margin:0 auto" to center horizontally you can use "margin-inline: auto". This way you do not initiallize margin in Y direction to 0.What if you already have some margin top and bottom .

  Good luck,

  Happy coding

  0