Frontend Mentor | Trevor Merrick's profile

All comments

@A-Fouad-CodeSubmitted about 3 years ago

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.

HTML and CSS only

Trevor Merrick•150

@tmerrick17Posted about 3 years ago

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

Trevor Merrick•150

@tmerrick17Submitted about 3 years ago

Great first try at this. I'm loving it a lot.

Mobile ready first page using BEM and accessibility in consideration

Trevor Merrick•150

@tmerrick17Posted about 3 years ago

Thanks for the comments. I really appreciate the feedback.

Things I fixed:

I took out the alt attribute after reading this article to better understand what you were saying. https://supercooldesign.co.uk/blog/how-to-write-good-alt-text I appreciate this. I will do this going forward with more “decorative” images.

Things to (kindly) push back on:

In the syle-guide.md file it specifies the desktop width to be 1440px on line 8. This is simply where I added the pattern-background-desktop.svg to the background. You are right though, I would have added the desktop background probably closer to 1200px but I want to be a precise as possible with the documentation. Thoughts on this?

Questions on:

In the very beginning of you comments you mention “…background-wave is not really taking the whole width like it’s supposed to.”

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!