@A-Fouad-Code
Submitted
If you have any comments or ideas on how I can make my code better or the design look better please share your thoughts with me.
Looking to hire developers?
@tmerrick17
@A-Fouad-Code
Submitted
If you have any comments or ideas on how I can make my code better or the design look better please share your thoughts with me.
@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:
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;
}
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
@tmerrick17
Submitted
Great first try at this. I'm loving it a lot.
@tmerrick17
Posted
Thanks for the comments. I really appreciate the feedback.
Things I fixed:
Things to (kindly) push back on:
Questions on:
Both mobile and desktop wave backgrounds go across my browser all the way. I’ve tested out on: Operating System macOS High Sierra Version 10.13.6 - Firefox 92.0 (works like the solution displays) - Chrome 93.0.4577.82 (works like the solution displays) - Safari 13.1.2 (wave background doesn’t show up at all…will check into this.)
What browser is this happening to on your end?
Most importantly I really do appreciate all the feedback. I’ve been coding for just about a year now and Frontend Mentor has already been a great find!