Challenge 3 column

Community feedback

• 

  Said Alejandro Rosas Vera•145

  @said-alrovePosted over 3 years ago

  Hi @Carlos! a few things to say:

  ✅.- You declared Grid properties in your main container without needing them due to you didn't declare display Grid in there, instead, you declared Flexbox.

  ✅.- Either Flexbox or Grid is unnecessary for the mobile view because it only needs 1 main column at that point, and the div element has a display block thus you won't need to wrap them to the next row manually.

  ✅.- Because you didn't assign a width value to the main container, its size it's relative to the padding you set in the body which may not be a problem in desktop or even tablet view, but in mobile, it provokes that the container gets too much small, what I recommend you is to set three values ​​to the container's width with the clamp () function which takes a minimum, a fixed, and a maximum value for better control over the layout of your container, I'd put this clamp (37.5rem, 90%, 110rem) ... that way your main container it's gonna take a 90% of the total size (what combined with margin: 0 auto; for center it, could make that same as the padding you declared in the body), but at the same time, that 90% can not be less than 37.5rem or more than 110rem. If you want more information about the clamp function, check this article.

  ✅.- In like manner I saw that you didn't use rems as your measurement units, therefore if you don't what I'm talking about when I say "rems" you can check this other article, otherwise if you already knew about it but you didn't want to use them in the challenge, I highly encourage you to use them due to they're relative units and not fixed as pixels.

  ✅.- The button should have the background color transparent just when it's active, not when it's hover, you might use .button:active instead of .button:hover.

  ✅.- To declare Flexbox in the buttons it's unnecessary, same as for the other Flex properties like align-items and justify-content, therefore you should remove them.

  ✅.- As I saw, you declared a specific class for each button, which could be great to give it its special text color without too much trouble, but I encourage you to create two classes for each button, the one you've already declared in them, and one class that the three buttons share, that way you can declare the properties that the buttons have in common (the padding for example), and just when you need to be more specific with a button in special, you can use its own unique class (for the text color for example).

  ✅.- The same as up, but with your columns, you should create a class where you can set the properties that the three columns share, and use the unique class to declare specific things such as the background-color.

  ✅.- With respect to the border-radius I highly recommend you to declare it in your main container instead of each individual column, because doing it that way, you'll have to change the property in order of the main container layout. If you want to do it, plus to what I said you, add the property "overflow" and give it a value of "hidden", that way the main container will have rounded corners no matter what (If you don't do it, because the other columns don't have the same corners, they're not going to "respect" the border-radius of the main container).

  ✅.- You used the bottom and left properties in the attribution container without declare the position absolute to it, do it, and that way your attribution container will be positioned in the bottom with respect to the body. In like manner, because you give it a width of 100%, you don't really need to specify the left property, because no matter from where it starts, it's gonna fill the 100%.

  ✅.- The paragraphs already have a font-weight of 400, so you don't have to specify it. Likewise, you declared the text-align property with an "initial" value which it's unnecessary (unless you first change the text-align from its default, and then you want to return it to its original value).

  ✅.- Your image and your title don't really need a display of Flexbox, therefore you can remove it from them.

  ✅.- There are several properties that you declared in your media query that you had already declared in your normal stylesheet, remember Don't repeat yourself. In like manner I encourage you to use min-width instead of max-width in future projects due to max-width it's focused on a desktop-first concept, which is that you start making your layout for desktop first, and then with the max-width media queries you continue with the mobile layout by overwriting properties that you had already declared; with min-width what you do is to start writing properties for the mobile view (mobile-first) in your normal stylesheet, and then you start using the media queries for bigger screen-size devices, that way you can have more control over the properties you declare.

  I hope this could help you to improve your skills, have a nice day :D!.

  Marked as helpful

  0

  ---

  

  Carlos Eduardo•160

  @carlosecosmesilvaPosted over 3 years ago

  @said-alrove

  Thanks for all the tips I'll keep improving and I'll restart the project using the mobile first system as a base.

  0

  

  Said Alejandro Rosas Vera•145

  @said-alrovePosted over 3 years ago

  @carlosecosmesilva Good luck, have fun coding!

  0