Final CSS layout seal of approval request

HTML & CSS
1

Hi from 12 degrees C cloudy York UK :slight_smile:

After tonnes of help from Sitepoint Members I am very gratefull to have given my site a css layout makeover www.davidclick.com

But before I pop the champagne the devil lies in cross browser compatibility so my question is please can anyone spot a significant layout CSS jinx that I may have overlooked.

Thanks in advance,
David

2

Hi David,

If you want an actual review then you will need to post in the review section once you have completed the requirements.

I’ll leave this thread open but comments should be directed at bugs and code issues and not the design itself as that is what the review section is for :).

I’ll start the ball rolling:

Why the transitional doctype and not strict ?

Why the extra navigation div around the ul? The ul is a perfectly good container and you don’t need a div around it as well. Just apply the styles that you had on the div and put them on the ul.

3

You know what? This is a perfect thread to use for “Code Review” if one ever gets started for HTML/CSS. I don’t believe Zygoma was asking for a “how does it look” website review at all.

Code comments (not really CSS comments):

<meta http-equiv=“Content-Type” content=“text/html; charset=UTF-8” />

Make this the first thing in the <head>. Read why.
Personally I do:
charset/content-type
language if I also include a lang meta
title
all other junk

In the images portfolio, you’ve got titles on stuff. Titles only appear to those with mice… how important is that information? Consider captions.
Conversely, the alt text is nasty. I would put what you currently have in your titles into your alt attributes, then remove the titles. But that’s personal. To stop IE from showing the alt text on mouseover, you can have title=“” instead.

Titles on links, though, especially on links that have their own link text, is not only redundant but for some people reduces usability (as every time they try to click on the link they get this tooltip in the way… a tooltip that doesn’t tell them anything useful).

There are three links to the same page (the prices/packages link). Each link claims (in its link text) to go to something different, when in fact they all go to the same place. The way to fix that? Either a single link to “prices” in general (with contextual surrounding text mentioning there are packages) OR using URL fragments so each link actually DOES go somewhere different: specifically to the section you want to link to.

prices.html#pkg1
where then the box containing the first package has an id of pkg1… and so on.

Ok, CSS


#header a, 
#header a:link 
{

Unless you’re doing something goofy with the name attribute, the a:link is kinda redundant: it’ll inherit from #header a by itself automagically.


#header h1

Got any other h1’s anywhere? “h1” would be nanosmears faster than #header h1, since the browser parser has to find the h1 and then continue on checking if it’s indeed in a #header element.

As Paul said, you don’t need a wrapper div on the ul the way it is now. If you did remove the wrapper div, you’d be able to turn this:


#navigation
{
	float: left;
	width: 900px;
	background: #333;
}

#navigation ul
{
	margin: 0;
	padding: 0;
}

#navigation ul li
{
	list-style-type: none;
	display: inline;
}

into this


#navigation
{
	list-style-type: none;
	margin: 0;
	padding: 0;
	float: left;
	width: 900px;
	background: #333;
}
#navigation li
{
	display: inline;
}

and you can also take this


#navigation li a
{
	display: block;
	float: left;
...
}

and make it this


#navigation a
{
	float: left;
...
}

Less code, less there is to break.

What’s Content clearing?

Something really weird:
when I view source I see this:


    <div id="footer_[b]prices[/b]">5 Drummond House, College Mews, York, YO31 7SH | © davidclick.com, 2011.
    <div><img src="/images/david-click-web-logo_4.jpg" width="150" height="50" alt="davidclick.com" /></div></div>

but when I View Generated Source I get this:


    <div id="footer_[b]venue[/b]">[b]<p>[/b]5 Drummond House, College Mews, York, YO31 7SH | © davidclick.com, 2011.[b]</p><p>
    </p>[/b]<div><img src="/images/david-click-web-logo_4.jpg" alt="davidclick.com" height="50" width="150"></div>
</div>

I dunno why the id’s are different… I suspect the p’s are added by FF because it doesn’t like
<div>
plain text loose <div>inline</div>
</div>
but I’m not sure.
I wouldn’t wrap an image in a div anyway. You can either
<div>
plain text loose <img>
</div>
(and float the image right)
or
<div>
<p>plain non-loose text</p>
<p><img></p>
</div>
Or leave the image out, since the only reason for the second p is to prevent an inline sibling next to a block, which doesn’t matter bug-wise because you’re making the img render as a block by floating it anyway…
I don’t believe that img IS a paragraph so I’d be reluctant to do that.

Or both text and image in inline boxes such as spans.

#footer
{
	clear: left;
	background: #ccc;
	text-align: right;
	padding: 20px;
	[b]height: 1%;[/b]
}

If you’re triggering Haslayout, I’d set a width (minus the padding of course) since 1% height can be taken literally by any browser who isn’t IE6.

If you have different styles for different footers, why not this?

#footer {
basic styles ALL footers have;
}
#footer.venue {
styles for a footer with also the class of venue;
LIST ONLY WHAT’S DIFFERENT FROM #FOOTER
}
#footer.prices {
styles for a footer with also the class of prices;
LIST ONLY WHAT’S DIFFERENT FROM #FOOTER
}

<div id=“footer” class=“prices”>footer footer</div>

This rather than all classes because of IE6 being retarded with multiple classes, plus I like id’s on my major boxes : )

4

Thanks guys… yes I think a code review section / crtique would be a great idea. Reading Stommes review has been very interesting.

5

Lawlz, I love your little images.

I made a typo:

not image