Feedback Anyone?

I’ve been working on this project for a few weeks now. I used bootstrap4 for this project. I am very pleased with how it looks. I’m sure it could be better, so if anyone has any thoughts I’m happy to hear.

@astv99 check out my page speed insights I used yall.js to lazy load my images and map. I probably still wouldn’t know about page speed insight if you didn’t show me, so thank a million man. From that link you gave me I’ve learnt so much from just following the other links on that page. I still have to optimize my images, I usually do this manually but I think i’ve done it enough by hand to have at least a decent appreciation for what it entails. So I’m looking for a tool to automate the process now. Maybe this is where I start learning to use Gulp.

@AdityaVT I used the parallax code I made when I first started FCC, I had to tweek it but it worked!!

I plan to donate this to a local business when it’s done. Here’s the project link.

4 Likes

Be mindful of overflowing content. The labels in the content panes are crashing (my innerWidth is 1140px) :

2 Likes

Thank you @snowmonkey, I made the font smaller it looks a lot better that way. That was a good catch.

Truly do like the look, and the subtle parallax going on when I scroll the page? Nice touch.

1 Like

That parallax was a fun project that I took on early own when I was trying to understand 3d transforms. It was really challenging but it taught me a lot. I do hope the business owner feels the same way as you do.

Really nice work, looks great! I like the modern design, and the code looks good too. Overall a really good improvement from the last project that you posted here. :wink:

Just a few things to note:

  • The W3C Validator is reporting a bunch of errors that should be fixed.
  • I’d recommend allowing the height of your initial “jumbotron” image to extend lower more. As it is now, you have a 150-pixel beige-colored area below the image that’s not very aesthetically pleasing. It’d look better if the beige-colored area was either shorter (50 pixels at most) or just not visible at all on initial load, which you could do by allowing the image to fill the viewport height.
  • It wasn’t obvious that you had placeholder text until I saw it repeated in various places. Whenever you need placeholder text, you should use lorem ipsum: https://www.lipsum.com/
1 Like

@astv99 thanks for the input. I fixed the validation issues, made the “jumbotron” image cover the screen and I replaced the text with lipsum text.

Thanks, I had to start over two times because I didn’t like the look. Design is the most frustrating part for me, it’s not the part I’m interested in. The coding is my favorite but I know my designs should at least look clean so I still try to work at it. I plan to use the little bit I’ve learnt to redesign the last project I posted, make it cleaner.

.p.s. I’m practicing my JavaScript. I didn’t have to do much here but I did write some code to toggle the shadow when I scroll up or down. Could anyone please let me know if their is anything I should fix. I’m thinking along the lines of any best practice or convention that I’m not using. Thanks

Glad to see you incorporating your old experiments into your new projects. Eager to see what you will come up with next.

1 Like

excellent and professional design. well done.

i’ve only a single nitpick mainly because the site is so good

  • your request for quote buttons could use their text centered. currently they are floating a bit to the top of their boxes
2 Likes

Thank you as always for the encouragement @AdityaVT. @rekurzion that was a good catch. I fixed it, buttons are nice and centered thank you.

1 Like

Looks pretty good.

  1. I would remove the min-height on the navbar. You can give some more padding instead to get the hight you want. This way the mobile nav will open up without pushing the logo and burger icon up.
.navbar--main {
  /*min-height: 12vh;*/
  font-size: 1rem;
  background-color: white;
  padding: 1.5rem 1rem; /* You may need to adjust it in a mobile media query as well  */
}
  1. I like that you have different utility functions.

I would wrap all the code in a function, either just an IIFE or a DOMContentLoaded event handler (or an object, but then you have to refactor).

You might as well use const instead of let for your functions and the object (it is even called CONST but is declared using let).

Maybe look into throttling the scroll event.

  1. The page does have a bit of a contrast problem. The yellow is fairly bright against the white background. I guess it might be a brand color, if so, changing it is clearly not an option. However, you might consider using a very slight gray for the background color and maybe try some background color for the values section to make the icons stand out better.
1 Like

Really nice design! As others have said the parallax is great - well done for putting so much effort into this!

1 Like

@lasjorg wow thank you for all the pointers. I was just learning about IIFE, I used it on a JavaScript clock I’m working on, can’t wait to show you guys that one. I have no problem refactoring, will get to it as soon as. I’m working on 3 projects at the moment, so I’m trying to split my time between them. Gotta make the best of the 24. I will look into all the improvements you have suggested. Thank you for taking a look at under the hood too, The code is the part I need the most mentoring on. I’ll let you know as soon as I’ve made the improvements, thank you.

thank you @simongreenuk. I try my best and I’ve learnt so much from the people here on the forum.

Looks really good. The only thing that I could see that would need fixing is on your about page. The last letter of your last name, “O”, bleeds into the next section and should have some black in between.

Other than that, awesome job.

1 Like

Thanks for all the pointers @lasjorg. These are the changes I made:

Removed the min-height like you suggested and it worked, stopped the logo and the hamburger from jumping.

I wrapped my code in an IIFE and removed that ugly “CONST” object. Used const instead.

I implemented a throttle function. I was going to use underscore.js but since I’m just learning I wanted to understand what was going on under the hood so I decided to try to implement a throttle function of my own. In future projects I will start using underscore.js though.

I tried the light gray background like you suggested, but I didn’t like the look so I tried giving the SVGs a dark stroke color to kind of break the contrast. It seems to work, the contrast doesn’t look too much anymore.

Thank You for the insights. It was a fun challenge and a great learning experience.

Looks better.

  1. The backhoe image is losing its aspect ratio, I would try background-size: contain; on it.

Don’t think you need the two below:

@media (min-width: 768px)
.section--services.lazy-bg-loaded::before {
    background: url(../img/backhoe.jpg) no-repeat 0% 60%/95% 95%;
}

@media (min-width: 425px)
.section--services.lazy-bg-loaded::before {
    background: url(../img/backhoe.jpg) no-repeat 0% 60%/cover;
}

And just use this:

.section--services.lazy-bg-loaded::before {
    background: url(../img/backhoe.jpg) no-repeat center/contain;
}
  1. As for the code, the things I said in the PM about the BEMBlock and only returning the init method applies here as well.

  2. I think 1 sec delay is to long on the throttle. I would also add a transition to the box-shadow

.navbar--main {
    font-size: 1rem;
    background-color: white;
    padding-bottom: 1rem;
    transition: box-shadow .2s ease-out;
}
1 Like

Thanks mate. I fixed the stuff you pointed out. I’ve already fixed the portfolio too, I haven’t merged it into my master branch yet though.Thank you for turning me onto “Prettier”. I must say, you seem like you have a lot of experience. I’ve learned quite a bit from my brief interaction with you. I will be as good as you some day. Thank you.

Trust me you’re doing great.:+1:

1 Like