Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on my first HTML5/CSS web site and, like all of my first-time projects, they end up cumbersome, crude, and hard to work with when changes need to be made later in the life cycle. I am attaching some code with a small amount of HTML and CSS. I have read up on CSS and HTML and, after quite alot of effort, have ended up with this. Please look at the code and fill me in on any inefficiencies that I may have introduced due to lack of experience. I do feel like I am doing a little bit too much manually right now and this is just a basic web site. Any opinion will be appreciated. FYI: I have left the colors a bit funny in order to easily see changes to the web site.

HTML

<html>
    <head>
        <title>Site Title</title>
        <meta charset="UTF-8">
        <meta name="viewport" content="width=device-width, initial-scale=1.0">
        <link rel="stylesheet" href="css/basecss.css"/>
    </head>
    <body>
        <div id="header">
            <div id="logo"></div>
        </div>
        <div id="listheader">
            <h1><u>List of Web Sites Header</u></h1>
        </div>
        <div id="site1Container">
            <div id="site1logo"></div>
            <div id="site1desc">Site description goes here.</div>
        </div>
        <div id="site2Container">
            <div id="site2logo"></div>
            <div id="site1desc">Site description goes here.</div>
        </div>
        <div id="site3Container">
            <div id="site3logo"></div>
            <div id="site1desc">Site description goes here.</div>
        </div>
    </body>
</html>

CSS

/* 
    Created on : Mar 24, 2014, 9:05:35 AM
    Author     : meggleston
*/
#listheader
{
    position:absolute;
    top:90px;
    width:100%;
    background:cadetblue;
    height: 80px;
    text-align: center;
}

#header
{
    top: 0;
    position: absolute;
    height: 90px;
    width: 100%;
    background: #007400;
    margin: 0px;
    padding: 0px;
}

#logo
{
    position:absolute;
    background-image:url(../assets/pics/logomain.png);
    background-repeat: no-repeat;
    background-re:no-repeat;
    background-position: left top;
    background-size:100px 90px;
    height: 100;
    width: 100;
}

body
{
    margin: 0px;
    padding: 0px;
}

#site1Container
{
    position:absolute;
    background-color: #cd0a0a;
    top:170px;
    width: 100%;
    height: 180px;
}

#site2Container
{
    position:absolute;
    background-color: bisque;
    top:350px;
    width: 100%;
    height: 180px;
}

#site3Container
{
    position:absolute;
    background-color: darkgoldenrod;
    top:530px;
    width: 100%;
    height: 180px;
}

#site1logo
{
    position:absolute;
    top:20%;
    background-image:url(../assets/pics/logo1.png);
    background-repeat: no-repeat;
    background-re:no-repeat;
    background-position: left top;
    background-size:100px 100px;
    height: 100%;
    width: 100;
}

#site2logo
{
    position:absolute;
    top:20%;
    background-image:url(../assets/pics/logo2.png);
    background-repeat: no-repeat;
    background-re:no-repeat;
    background-position: left top;
    background-size:100px 100px;
    height: 100;
    width: 100;
}

#site3logo
{
    position:absolute;
    top:20%;
    background-image:url(../assets/pics/logo3.png);
    background-repeat: no-repeat;
    background-re:no-repeat;
    background-position: left top;
    background-size:100px 100px;
    height: 100;
    width: 100;
}

#site1desc
{
    position: absolute;
    left:150px;
    top: 20%;
}

#site2desc
{
    position: absolute;
    left:150px;
    top: 20%;
}

#site3desc
{
    position: absolute;
    left:150px;
    top: 20%;
}
share|improve this question
2  
One quick note, you can save yourself a lot of layout headache, if you leverage a CSS framework like Twitter.Bootstrap (getbootstrap.com/2.3.2) it is drastically easier to create responsive layouts with a lot less "Cross-browser" issues, as they've already done a lot of the heavy lifting for you. Also, as stated, don't use absolute positioning, it's almost never necessary/a good idea. –  Mitchell Lee Mar 26 at 16:53
2  
@MitchellLee Bootstrap isn't part of Twitter anymore, also there is Bootstrap 3 now: getbootstrap.com –  RoToRa Mar 27 at 9:35
2  
Using Bootstrap is also a really good way to bloat your markup. I wouldn't recommend its use to anyone. –  cimmanon Mar 27 at 13:48
    
That's why it offers the different css components as separate .less files. You can pick and choose the elements you need (e.g. just the grid system) and ignore the rest. Combine + minify your css and there is negligible increases in bloat. –  harryg Mar 27 at 17:23
    
If you are wanting to learn then it is arguably better to start off without using a framework. –  w3d Mar 28 at 20:15

5 Answers 5

up vote 30 down vote accepted

Semantically, I would suggest using HTML5 elements more.

For example, instead of...

<div id="header">
    <div id="logo"></div>
</div>

Use instead: (the ID can stay if you want it to)

<header>
    <div id="logo"></div>
</header>

This would change your CSS from #header {} to just header {}

It is common to see multiple tags in complicated templates. So something like this might be appropriate if that is the case...

<header class="site-header">
    <div id="logo"></div>
</header>

CSS would be:

header.site-header {}

Consistency, consistency, consistency.

Most of your class and ID names are completely lowercase. However a few classes use camelCase like: #site3Container

Make your naming conventions consistent. Either use all camelCase or use all lowercase or all UPPERCASE etc.

Typically, I use lowercase for HTML and CSS (faster to write). I use camelCase for PHP. This is no rule however.


Positioning

Absolute positioning has its uses. It's almost never used for basic layout however.

Your exact layout can be achieved without the use of ANY position: absolute;. It is my general recommendation to avoid position: absolute; at almost all times. If you are finding you need to use it for something, chances are you don't actually need to, you just don't know how else to do it yet. Consult someone.

Here is a JSFiddle of your current html and css

Here is a JSFiddle demonstrating the same layout, without positioning.

*Note that the description text now has a maximum width of 960px (common site width, though 1140 is becoming more common). The description divs are also center so there will always be equal space on left and right sides.

My modifications above also feature one particular additional CSS selector:

* { margin: 0; padding: 0; }

The asterisk * is a wildcard, it essentially means ANY element. The selector above resets the margin and padding of every element to 0. This is usually done via something called a "reset stylesheet". Which leads me to my next subject...


Reset Stylesheets

Reset stylesheets contain CSS code which standardizes what your website will look like across all browsers. It sets all padding and margins to 0 for instance, so that no default styling is added to your site that you potentially, or even probably, don't want.

A quick Google search lands us at this website: http://www.cssreset.com/

There, you can see many popular reset stylesheets, all with their own different sort of caveats and specialities. I personally recommend Normalize.css It keeps and standardizes some typical styles you may not necessarily want to reset to 0. It also contains some standard typography styling, so your text looks great without you having to do any work. Reset stylesheets should always be the very first .css file you link. So you would include yours like this for instance:

<head>
    <title>Site Title</title>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="css/reset.css"/>
    <link rel="stylesheet" href="css/basecss.css"/>
</head>

Classes over ID's

An ID is specific, they can be used once. It IS this "thing" and it is only ever this "thing". A class can be just as specific, or as broad as you would like. A class can be applied to multiple elements on the same page. An example, expanding from the JS Fiddles above, would be to combine the styles for the description divs.

We start with this CSS:

#site1desc
{
    width: 100%;
    max-width: 960px;
    margin: 0 auto;
}

#site2desc
{
    width: 100%;
    max-width: 960px;
    margin: 0 auto;
}

#site3desc
{
    width: 100%;
    max-width: 960px;
    margin: 0 auto;
}

By modifying the HTML so that every <div id="site1desc"> is now this: <div class="contentdesc"> We can drastically cut down on our total CSS code. Our full HTML now looks like the below...

<div id="site1Container">
    <div id="site1logo"></div>
    <div class="contentdesc">Site description goes here.</div>
</div>
<div id="site2Container">
    <div id="site2logo"></div>
    <div class="contentdesc">Site description goes here.</div>
</div>
<div id="site3Container">
    <div id="site3logo"></div>
    <div class="contentdesc">Site description goes here
    </div>
</div>

The only CSS we need changes from the 3 selectors above to just this:

.contentdesc
{
    width: 100%;
    max-width: 960px;
    margin: 0 auto;
}

JSFiddle Demo of the above

***Note: CSS classes are referenced with a period .className is different from #idName

share|improve this answer
2  
Nice answer thanks for chiming in! –  Malachi Mar 26 at 16:04
1  
CSS resets are nothing but snake oil. Setting all of your margins to 0 just so you can set them to something sane elsewhere is stupid. –  cimmanon Mar 27 at 13:50
3  
Are you kidding me? There are plenty of good reasons to utilize a css reset. Good ones like Normalize do a hell of a lot more than just reset padding and margin spaces. Normalize in particular doesn't set to 0 anyways, but sets to pretty common spaces used by a number of modern sites. –  Michael Mar 27 at 13:56
    
This is a shining example of what kind of answers SO is looking for. –  JoshWillik Mar 27 at 19:12
    
TYVM @JoshWillik –  Michael Mar 27 at 19:16

One quick remark: Never leave out the DOCTYPE (<!doctype html> for HTML 5), unless you want to work around the bugs that the browsers will emulate.

EDIT: I forgot the "5" in HTML 5.

Also: It doesn't really matter which DOCTYPE you use, as long it triggers Standards Mode the browser. Browsers don't differentiate between HTML versions. You could use an "old" HTML 4 DOCTYPE but write HTML 5 code or use the HTML 5 DOCTYPE and just write "plain" HTML 4. As long as the browser is in Standards Mode it will be fine.

share|improve this answer
    
for HTML5 or for any HTML? –  Malachi Mar 26 at 16:28
5  
For any HTML, although if your not using HTML5 you should add the correct doctype (XHTML Transitional, strict, etc) –  Mitchell Lee Mar 26 at 16:50
    
@MitchellLee thanks. most of the time I let my IDE create the DocTypes and don't pay any attention, shame on me.... –  Malachi Mar 26 at 16:53
    
@Malachi if your IDE is adding the doctype to the page, then it is part of your outputted HTML, and that's an acceptable solution. The main thing is the browser has to see the doctype, as it will make a lot of weird issues (especially with IE6-8) go away. –  Mitchell Lee Mar 26 at 16:55
2  
@Malachi these days, I'd say you should just stick to <!doctype html>. HTML5 is the most advanced, and well supported by almost every browser these days, and due to the nature of older browsers being (almost TOO forgiving) malformed HTML, it generally doesn't cause any problems there. Combine it with a CSS Framework like Boostrap, and Modernzr javascript library, and other polyfils, it's the path to least resistence. –  Mitchell Lee Mar 26 at 17:15

One example of where you should use other HTML Elements

    <div id="site1Container">
        <div id="site1logo"></div>
        <div id="site1desc">Site description goes here.</div>
    </div>

This should look like this

    <div id="site1Container">
        <img id="site1logo" src="http://www.google.com/yourpic.jpg" alt="funny picture"/>
        <p class="site1desc">Site description goes here.</p>
    </div>

You were also using the same ID attribute multiple times, which is a no-no, use classes for something where you are going to use a similar Style for multiple tags.

you might want to lose the ID on the img tag as well, if you are going to display a picture then display a picture not a background.

I only pointed out one instance, there are others in your code


Don't use old outdated tags like this one

<h1><u>List of Web Sites Header</u></h1>

if you want to underline something, use a style

<h1 style="text-decoration:underline"> List of Web Sites Header </h1>

if you are going to use the same style in more than one place use a class, only write inline style rules where absolutely necessary, they can clutter your HTML

share|improve this answer
2  
You probable wanted to use src attribute for img tag, not href. –  Cthulhu Mar 27 at 12:40
    
yes, I had a brain fart. thank you –  Malachi Mar 27 at 12:42
    
Additionally dropping the class on child elements will promote css's natural cascade. E.g. specificity vs implied. And my personal preference is to use classes for styling and id's for functionality as id's take precedence over classes, Pseudo elements, and selectors so avoiding using ID's for styling will make it more scalable as a whole. –  darcher Mar 28 at 12:49

The CSS rulesets for your #site1desc and #site2desc etc. are all the same (copy-and-pasted). Instead there could be just one class class="sitedesc" and CSS selector .sitedesc; or several ID selectors for the same ruleset

#site1desc, #site2desc, #site3desc
{
    position: absolute;
    left:150px;
    top: 20%;
}

Your #site1logo and #site2logo etc. definitions are all the same, except for the URL of the logo. You could make these all the same class with one ruleset; and differentate just the logo with a rule like:

#site1Container > .sitelogo
{
    background-image:url(../assets/pics/logo1.png);
}

Similarly our #site1Container and #site2Container etc. rulesets are all the same, except for the background-color.

So I'd suggest HTML like:

    <div id="site1" class="siteContainer">
        <div class="siteLogo"></div>
        <div class="siteDesc">Site description goes here.</div>
    </div>
share|improve this answer

You should add the DOCTYPE at the top (before the html element):

<!DOCTYPE html>

The meta-charset should come before the title (so the encoding is known before parsers reach it):

<meta charset="UTF-8">
<title>Site Title</title>

If this is a page from a whole website, and not just a stand-alone page:

Your site title (which seems to be represented as logo in your case, so the textual title would be the alt value of that logo) should be a h1.

The main content (which currently has an h1), should be in scope of the site title, i.e., h2. Or even better, use a sectioning content element (section resp. article) for your main content. So the structure would look like:

<body>
  <h1><img src="logo.png" alt="Site title"></h1> <!-- don’t append "logo" to the alt value -->
  <section> <!-- depending on your content, this could also be 'article' -->
    <h1>Main content title</h1>
    <!-- here comes your whole main content of that page -->
  </section>
</body>

You can read more about this in my other answers:


The u element doesn’t seem to be used correctly here ("unarticulated, though explicitly rendered, non-textual annotation"). If you want to display this text as underlined, use CSS instead:

#listheader h1 {text-decoration: underline;}

But note that in the Web underlines are typically associated with hyperlinks.

share|improve this answer
    
I disagree with your use of the H1 tag to surround the IMG tag. you will create padding and newlines and margins by doing this. an H1 is meant for text Headings not to encapsulate a Graphic, the graphic could be encapsulated by fancy HTML5 tags like <header> that would make more sense. and as far as everything else, it's all been said except for the Meta Tag stuff, which is a good point –  Malachi Mar 27 at 15:56
    
@Malachi: The point is that the site needs (*) a heading (for pages that are part of a website, that is; for example those with a global navigation). Whatever the site title may be should be enclosed in h1. It may be text, a logo or even an audio file, this doesn’t matter. -- And using a heading is about the semantics/accessibility, not about the default CSS styling it may have in browsers. -- (* It can also be omitted, but then the main content must be inside a sectioning content element, and the body sectioning root must not have other direct headings.) –  unor Mar 27 at 16:04
    
I see the Title inside the Title Tag nested in the header tag, where it belongs. like I said HTML5 has a tag for header sections of websites <header> and </header> that should surround the Logo and any H1 text that goes with it (separately) –  Malachi Mar 27 at 16:08
1  
you are over complicating things I think.. –  Malachi Mar 27 at 17:31
1  
Where the img tag is now valid in an h1 in html5, it isn't considered semantically sound. Either an image with an alt OR a h1 if your site name is the heading of that particular page. No text in an h1 makes little sense, just because you can do something doesn't mean you should. Additionally, the title tag and h1 on the page should compliment the page contents and each other, not replicate each other. A logo is site wide branding whereas title/h1 is page specific. –  darcher Mar 28 at 12:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.