SASS and Flexbox

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted over 3 years ago

  Hey, good work on this one. Layout in desktop seems pushed a little on the left side, mobile state, I cannot see any, nothing is appearing.

  Suggestions would be:

  1. I think you could have done this without using too much elements, you could just make the main the parent of those 3 cards and that really saves you some elements.

  2. Since on the desktop it is somehow pushed on the left side, adding display: flex; justify-content: center; to the row selector will make the cards centered properly. But I think, using float on those cards is not ideal, it is commonly used when you want an image to be somehow being wrapped by text, using float on it. But still, you somehow made it work and it's awesome.

  3. You need to refactor the mobile state. Right now, nothing is only appearing on the screen. This was caused by that massive margin on the cards selector. Doing the mobile state will be really good.

  4. Lastly, making it a habit of using 1 h1 per page will be really good. We want our markup to be semantic. You could use another header tags like h2 and make the h1 sr-only or some sort.

  But still, good job on this one^

  0

  ---

  

  Cassandra Dauphin•10

  @CalebDauphinPosted over 3 years ago

  @pikamart thank you for the advices I've updated it. Let me know how I did this time

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 3 years ago

  @cassandradauphin Hey, the layout got bigger and this time, you used height with vh units, which I always recommend NOT using. This is really hard to handle. You can see that if you inspect your layout, using dev tools from the bottom, the layout's size changes.

  This is because, it only takes a percentage base on the remaining screen's height. Instead of those, use other units like rem. Your first submission I think didn't use those vh units and I don't know why you did it this time. But just remember that vh units is not really good, well for me.

  You could remove both the height declaration in the row and card selector. Then if you could resize the cards to be a bit more smaller , that will be really good. Also you made two breakpoints which is confusing, you can look at other solution of other dev in here, that tackles this challenge. Good luck and thank you as well ^

  0

  

  Cassandra Dauphin•10

  @CalebDauphinPosted over 3 years ago

  @pikamart hey sorry to bother you again. i took me a while but i simplified the solution. please let me know what you think. am i heading in the right direction? or is it ok the way it is now?

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 3 years ago

  @cassandradauphin Hey, glad to see that you still refactor it, that is really good of you btw.

  Upon checking it now, it looks really good and the mobile state is good as well.

  You could try adding some cursor: pointer to elements that are interactable, just to make it more natural right.

  Also, I saw that you used href on the button, well it won't work. It needs to be inside a tags not button.

  Lastly, this is my personal preference. I like if the cards are more wider in the mobile state, like it will occupy 90% of the width but have that min-width. Just to occupy some white spacing. But that's on me.

  But still, this last one was really good and really, good job on this refactoring challenges^^

  0

  

  Cassandra Dauphin•10

  @CalebDauphinPosted over 3 years ago

  @pikamart THANK YOU!

  0
• 

  Sodeeq Olaide Awoyemi•70

  @ErrytagedesignPosted over 3 years ago

  I think your break point for the mobile is a little bit off, when view on mobile it doesnt display properly

  0

  ---

  

  Cassandra Dauphin•10

  @CalebDauphinPosted over 3 years ago

  @Errytagedesign thank you for letting me know

  0