Review for updated site

[linton]

I will appreciate reviews on my updated site w ww w lookupyoursite.com.

Basically, I wanted some practice with page layout and drop-down menus so I decided to do something new with my old practice site.

Watching CSS videos and reading etc does not yield results unless you practice.

I have validated the HTML and CSS and corrected the errors with the exception of one CSS error that I will look into later.

I really learned the importance of 'vanishing bullets', filling out your spaces (%20 error) and probably installing a code validator on your PC or putting up your site and checking for errors as you work.

I have a lot of 'text filling' on a few pages and I will work on those later due to time limitation.

 

Thanks.

[Andrea]

I've never installed any validator to my PC, I just use FXs Webdeveloper html validator - for your index page, btw, it shows 1 error.\: http://validator.w3.org/check?verbose=1&uri=http%3A%2F%2Flookupyoursite.com%2F

 

Looking at your site, I have the following comments:

 

The repeating bread/grain background image is very much early 90s and somewhat (ok - a LOT) tacky.

 

A page's title should be something meaningful and SEO relevant (not 'index') for this, you'd be better of using something like "Our Breads Bakery, Madison, VA - or something else relevant that someone might google when looking for good bread in this area.

 

When I click the hyperlink 'Grandma' - I really would not expect to send her an email in this context.

 

 <li><a href="[url="view-source:http://lookupyoursite.com/home/contact%20us.html"]home/contact%20us.html[/url]">Contact Us</a></li>

The "%20" tells me there is a space in your file name: Bad Practice!!! Use camel case or dashes - even _ are not the best solution, as they tned to disappear behind underlines.

 

In your CSS, your wrapper does not need to be relative. Relative means kind of taking an exception to where it normally would be - which does not apply here.

 

These are just a few issues I see on the fly. Let me know if anything does not make sense or if you have any specific questions about something else.

[linton]

I've never installed any validator to my PC, I just use FXs Webdeveloper html validator - for your index page, btw, it shows 1 error.\: http://validator.w3.org/check?verbose=1&uri=http%3A%2F%2Flookupyoursite.com%2F

 

Looking at your site, I have the following comments:

 

The repeating bread/grain background image is very much early 90s and somewhat (ok - a LOT) tacky.

 

A page's title should be something meaningful and SEO relevant (not 'index') for this, you'd be better of using something like "Our Breads Bakery, Madison, VA - or something else relevant that someone might google when looking for good bread in this area.

 

When I click the hyperlink 'Grandma' - I really would not expect to send her an email in this context.

 

 <li><a href="[url="view-source:http://lookupyoursite.com/home/contact%20us.html"]home/contact%20us.html[/url]">Contact Us</a></li>

The "%20" tells me there is a space in your file name: Bad Practice!!! Use camel case or dashes - even _ are not the best solution, as they tned to disappear behind underlines.

 

In your CSS, your wrapper does not need to be relative. Relative means kind of taking an exception to where it normally would be - which does not apply here.

 

These are just a few issues I see on the fly. Let me know if anything does not make sense or if you have any specific questions about something else.

 

Thanks for your comments and suggestions. Are you referring to the background image on the contact page when you say, "The repeating bread/grain background image is ..."

I couldn't figure out how to style that page with a different color so I decided to use a 'class' for that.

Edited March 12, 2012 by linton

[Andrea]

No, I meant the picture with the loaf of bread, bag of wheat, and the wheat itself.

 

I hadn't even looked at all the pages, but since you bring up the contact page:

 

The background you are using here is not really an image, it's just a color. The correct way to apply a color background is:

 

body (or whatever) { background: #D2A748;}

 

Creating a 'color' patch is unnecessary.

 

And when I looked the last time, I also overlooked how you coded your navigation. Your way works, but it's not what's recommended. this is how the html for your navigation SHOULD look like:

 

<div id="navMenu">
<ul>
 	<li><a href="index.html">Home</a>
   	<ul>
     	<li><a href="home/location.html">Location</a></li>
     	<li><a href="home/contact%20us.html">Contact Us</a></li>
     	<li><a href="home/credits.html">Credits</a></li>
   	</ul>
 	</li>
  	<li><a href="gallery/gallery.html">Gallery</a>
   	<ul>
     	<li><a href="gallery/fall.html">Fall</a></li>
     	<li><a href="gallery/summer.html">Summer</a></li>
     	<li><a href="gallery/winter.html">Winter</a></li>
     	<li><a href="gallery/spring.html">Spring</a></li>
   	</ul>
 	</li>
 	<li><a href="events/events.html">Events</a>
   	<ul>
     	<li><a href="events/old%20events.html">Old Events</a></li>
   	</ul>
 	</li>
 	<li><a href="forum/forum.html">Forum</a>
   	<ul>
     	<li><a href="forum/members.html">Members</a></li>
   	</ul>
 	</li>
 	<li><a href="links/links.html">Links</a>
   	<ul>
     	<li><a href="links/recipes.html">Recipes</a></li>
   	</ul>
 	</li>
 	<li><a href="catering/catering.html">Catering</a>
   	<ul>
     	<li><a href="catering/order.html">Order</a></li>
   	</ul>
 	</li>
</ul>
</div>

 

You just need ONE main list for all the main nav items, and then one additional list for each second level, but not an entire list for each item, as you have it.

 

Also, while the descriptions can be helpful at times, in this section, you almost have more comments than you have code, and things get cluttered.

 

And while this is not a rule, merely my opinion, I find that for example:

 

<div id="header"> by itself is already so descriptive that a comment of 'begin header' is totally redundant. However this:

<div id="header">
bla bla bla
</div> </-- end header -->

makes a lot more sense to me. (like I said, opinion, not law)

 

I'll look at your work again later on when I have more time.

[linton]

No, I meant the picture with the loaf of bread, bag of wheat, and the wheat itself.

 

I hadn't even looked at all the pages, but since you bring up the contact page:

 

The background you are using here is not really an image, it's just a color. The correct way to apply a color background is:

 

body (or whatever) { background: #D2A748;}

 

Creating a 'color' patch is unnecessary.

 

And when I looked the last time, I also overlooked how you coded your navigation. Your way works, but it's not what's recommended. this is how the html for your navigation SHOULD look like:

 

<div id="navMenu">
<ul>
 	<li><a href="index.html">Home</a>
   	<ul>
     	<li><a href="home/location.html">Location</a></li>
     	<li><a href="home/contact%20us.html">Contact Us</a></li>
     	<li><a href="home/credits.html">Credits</a></li>
   	</ul>
 	</li>
  	<li><a href="gallery/gallery.html">Gallery</a>
   	<ul>
     	<li><a href="gallery/fall.html">Fall</a></li>
     	<li><a href="gallery/summer.html">Summer</a></li>
     	<li><a href="gallery/winter.html">Winter</a></li>
     	<li><a href="gallery/spring.html">Spring</a></li>
   	</ul>
 	</li>
 	<li><a href="events/events.html">Events</a>
   	<ul>
     	<li><a href="events/old%20events.html">Old Events</a></li>
   	</ul>
 	</li>
 	<li><a href="forum/forum.html">Forum</a>
   	<ul>
     	<li><a href="forum/members.html">Members</a></li>
   	</ul>
 	</li>
 	<li><a href="links/links.html">Links</a>
   	<ul>
     	<li><a href="links/recipes.html">Recipes</a></li>
   	</ul>
 	</li>
 	<li><a href="catering/catering.html">Catering</a>
   	<ul>
     	<li><a href="catering/order.html">Order</a></li>
   	</ul>
 	</li>
</ul>
</div>

 

You just need ONE main list for all the main nav items, and then one additional list for each second level, but not an entire list for each item, as you have it.

 

Also, while the descriptions can be helpful at times, in this section, you almost have more comments than you have code, and things get cluttered.

 

And while this is not a rule, merely my opinion, I find that for example:

 

<div id="header"> by itself is already so descriptive that a comment of 'begin header' is totally redundant. However this:

<div id="header">
bla bla bla
</div> </-- end header -->

makes a lot more sense to me. (like I said, opinion, not law)

 

I'll look at your work again later on when I have more time.

 

I thought the 'picture' looked much better than a plain margin but I will go with your suggestion.

My plan was to give the 'contact page' a different background color from the other pages but I couldn't figure out how to do that without affecting the other pages plus I did not want to create a different style sheet/page for it at that time.

I will tone down on the 'comments' eventually, as a complete noob, it helps me find my way around plus it is like a new toy.

I do appreciate all your help especially when I look at what the first site I put up looked like.

Thanks.

[Andrea]

I thought the 'picture' looked much better than a plain margin but I will go with your suggestion.

 

That issue is very subjective, but I think I'm not the only one who is reminded of the website look of the early 90s - tiled background images just look somewhat amateurish.

My plan was to give the 'contact page' a different background color from the other pages but I couldn't figure out how to do that without affecting the other pages plus I did not want to create a different style sheet/page for it at that time.

You would not need a whole new stylesheet - you could just give the body of your contact sheet a different ID - for example,

<body id="contact">

and then add a style for #contact to your CSS. Example:

#contact {
	background: #D2A748

(Btw, #d2a748 is color of that color patch you used.)

 

Just make sure you add this AFTER the current body CSS.

 

Again: to make a background in any color, whether that's attached to your body tag, or any other tag, you do NOT need an image, you just use the color code. However, when you are using a background image - like your loaf of bread - you should also always provide a color code so the page is then displayed in the color you determine, should the image not me displayable.

 

I'm not saying to not use comments - they can be very helpful, just watch our for the overkill.

[linton]

That issue is very subjective, but I think I'm not the only one who is reminded of the website look of the early 90s - tiled background images just look somewhat amateurish.

 

You would not need a whole new stylesheet - you could just give the body of your contact sheet a different ID - for example,

<body id="contact">

and then add a style for #contact to your CSS. Example:

#contact {
	background: #D2A748

(Btw, #d2a748 is color of that color patch you used.)

 

Just make sure you add this AFTER the current body CSS.

 

Again: to make a background in any color, whether that's attached to your body tag, or any other tag, you do NOT need an image, you just use the color code. However, when you are using a background image - like your loaf of bread - you should also always provide a color code so the page is then displayed in the color you determine, should the image not me displayable.

 

I'm not saying to not use comments - they can be very helpful, just watch our for the overkill.

Thanks a bunch.