@tmerrick17
Posted
Hey Amr Fouad, first of all a great go at this project. Here is some feedback I would love to give you. I downloaded the files and took a deep dive so here it goes.
Things that worked well:
- CSS Reset is always a good idea. The basics you have exactly there.
- Custom CSS3 variables. Another good one that is so helpful, especially if you look at working in SCSS/Sass.
Things to ponder:
-
Best practice is to leave out unit (eg. 0 instead of 0px). This is nit-picky but a good standard to show other developers that you have the basics down.
grid-column-gap: 0px;turns into `grid-column-gap: 0; -
Definitely try to practice mobile first (look up Youtube videos, here's a good example.
-
Also when you hit 769px at that spot, the card is half out of the viewport. Try something around 1200px?
-
Try using px rather then % for
border-radius:. Start with 20px and go from there? -
What if you took off
.parent .div1? Try to not nest these already customized classes you've created. See what happens on the webpage, hopefully the answer is nothing. This could save you time writing code you might not need to type out.
Example:
.parent .div1 .txt h1 {
font-size: 1.6em;
padding-left: 0;
}
...turns into...
.txt h1 {
font-size: 1.6em;
padding-left: 0;
}
- Try using more descriptive classes then div2 but general like card-image? Too specific would be purple-image-of-people. Hopefully you get the idea.
Ok, I know that was long but I wanted to be as thorough as possible. You have done a great job at getting this far. Going back and tweeting while it might feel sucky, it means you are growing in your coding. So am I. I've only been coding for a a year now and really have enjoyed it. Hope to see more of your work!
Marked as helpful
