@GK-ProductionSubmitted over 3 years ago
This is a mobile-first layout and for the desktop layout I used Grid Layout for the cards. Again your feedback is highly appreciated and it will help me to improve for my future challenge here.
Said Alejandro Rosas Vera
@said-alroveAll comments
- @said-alrovePosted over 3 years ago
Hi! well done dude, just a few things:
✅.- You didn't import the font to the project, therefore it's using a default one, you can do two things here, import the font from your HTML, or from your CSS, the problem with the second option is that you can have a little bit of delay when loading the font due to the browser first renders the HTML (the content) and then renders the styles, thus you can have a weird moment when everything is done except your fonts, so it's your desition to choose from where to import your font. Here you have the font.
✅.- You declared the box-sizing property in your universal selector only, remember that you should declare it in both, your universal selector (with the pseudo-elements "before" and "after") and your "html" tag. For more information check this out.
✅.- I highly recommend you to order your CSS code in the following way: Globals (all your html tags, no classes here), utilities (here are classes that are shared by more than one element to assign it one or more properties, here your main container could stay), and then your normal styles (I separate this section by main components, in this case, you could put your Grid container altogether with the cards container, then your cards individually, and at the end your attribution). By the way, the way how I separate them is by commenting something descriptive like "MAIN-CONTAINER" at the beginning of the section.
✅.- You created two main containers, the first one is "grid-card-container", and the second one is "card", one of them is unnecessary, whichever you conserve, you can assign it all the styles there and everything will work perfectly (I'd assign to that container the "cards-container" class name" as a recommendation).
✅.- You used "translate" to position the side cards in the middle of the ones that are located in the half, as a recommendation ... I'd create a Grid with 4 rows, that way you can position the side cards from row 2 to row 4, and the ones that are located in the middle might be positioned from row 1 to row 3, and so on. Thanks to that you can have more control over your layout due to "translate" is something relative that could be not consistent.
✅.- Same as up, you can create a "middle design" to make it look better in the tablet view, you can check, as Patrick mentioned to you, my solution to this challenge, I basically inverted the design in the tablet view by creating a 3:4 Grid instead of a 4:3 as in the desktop view.
I hope this could be useful for you :D, have a nice day!.
Marked as helpful0 - @carlosecosmesilvaSubmitted over 3 years ago
Well this is my first challenge completed, I would like the community to help me on how I can improve, for example, if the code is too verbose or complex, feel free to give your opinion, all help is valid
@said-alrovePosted over 3 years agoHi @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 helpful0 - @ahtangSubmitted over 3 years ago@said-alrovePosted over 3 years ago
Hi, @ahtang nice one! 😸, just one thing ... I recommend you a lot to use comments to indicate where a div finishes, above all when you're using just divs for containers because it's a little hard to understand where a tag belongs to. The way how I do it is by commenting the class of the tag the element at the end of it (in the tag that closes it) and thanks to that you could have more readability in bigger projects.
Have a nice day :D!.
0 - @MuhammadLaeeqSubmitted over 3 years ago
Hi I really like this challenge as a newbie please share your valuable feedback to me Thanks :)
@said-alrovePosted over 3 years agoHi TechnoSolution! ✌
Well done with this challenge, just some advice:
✅ You can add the circles in the background by using the background property, nevermind if there's more than 1 image, you can control them easily, check this article for more information.
✅ Since you put 20% as width, there'll be problems with the mobile view, I recommend you to add a clamp() to the width, that way you can control thee values, a minimum (375px maybe), a default (here you can put 20%), and a maximum value (let's say 500px), check this article for more information. In like manner you can also add a min-width and a max-width to control just to values, you're free to choose either!.
✅ Add padding to the main container, that way on mobile view there won't be problems with the phone's borders.
✅ Same as up, you have to leave the alt attributes empty when there's not something important that the screen reader should read.
Have a nice day :D!.
0
