Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just started learning CSS/HTML a week ago and made a quick site today. It looks pretty good but I think I reused/wrote really messy CSS. This is because I haven't the "float" property really right so I keep using position:relative and top to offset the float.

Anyway, here's the site:

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
  "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">

<html xmlns="http://www.w3.org/1999/xhtml"
      xml:lang="en">
<head>    
  <title>Kevin Li</title>
  <link rel="stylesheet"
        type="text/css"
        href="css/style.css" />
</head>

<body>
  <div id="wrapper">
    <div id="header">
      <h1>Kevin Li</h1>
    </div>

    <div id="links">
      <ul>
        <li><a href="#">Home</a></li>    
        <li><a href="#">Biography</a></li>    
        <li><a href="#">Portfolio</a></li>    
        <li><a href="#">Blog</a></li>    
        <li><a href="#">Images</a></li>    
        <li><a href="#">Contact Me</a></li>
      </ul>
    </div>

    <div class="sidebar_left">
      Lorem ipsum dolor sit amet...<br />
    </div>

    <div id="post">
      <b>Introduction</b><br />
      <i>Thursday, January 27, 2011</i>
    </div><br />

    <div id="content">
      <img id="tree"
           src="images/c2_i6.png" />   
      <p>Lorem ipsum dolor sit amet...<br />
      <br />
      <img id="blackwhite"
           src="images/c3_i7.png" /> Ut venenatis diam nunc...<br />
      <br /></p>
    </div>

    <div id="footer">
      <b>Copyright 2010 Kevin Li</b>
    </div>
  </div>
</body>
</html>

And the CSS:

/*General Elements*/
body {
 background: #53777A;
 font-family: Garamond, Baskerville, "Baskerville Old Face", "Hoefler Text", "Times New Roman", serif;      
}

h1 {
 font-size: 28pt;       
}

h2 {

}

p {

}

a:link {color:black; text-decoration: none;}
a:visited {color:purple; text-decoration: none;}
a:hover {color:green; text-decoration: underline;}
a:active {color:yellow; text-decoration: none;}

/*Curvy Shapes*/
 #wrapper, #footer {
 -moz-border-radius-bottomright: 50px;
 border-bottom-right-radius: 50px;
 -moz-border-radius-topleft: 50px;
 border-top-left-radius: 50px;
 -moz-border-radius-topright: 50px;
 border-top-right-radius: 50px;
 -moz-border-radius-bottomleft: 50px;
 border-bottom-left-radius: 50px;
}

#links {
 -moz-border-radius-bottomright: 50px;
 border-bottom-right-radius: 50px;
 -moz-border-radius-bottomleft: 50px;
 border-bottom-left-radius: 50px;
}

#header {    
 -moz-border-radius-topleft: 50px;
 border-top-left-radius: 50px;
 -moz-border-radius-topright: 50px;
 border-top-right-radius: 50px;    
}

/*Structure*/
#wrapper {
 width:900px;
 margin: 0 auto;
 margin-top:30px;
 overflow: auto;
 background:#E0E4CC;
 padding: 20px;
 -moz-border-radius-bottomright: 50px;
 border-bottom-right-radius: 50px;
 -moz-border-radius-topleft: 50px;
 border-top-left-radius: 50px;
 -moz-border-radius-topright: 50px;
 border-top-right-radius: 50px;
 -moz-border-radius-bottomleft: 50px;
 border-bottom-left-radius: 50px;       
}

#header {
 text-align: center;
 background:#ECD078;
 padding: 4px;
}

#links {
 width:900px;
 background-color:#A7DBD8;
 position:relative;
 top:-20px;
 text-align: center;
}

#links ul {
 list-style-type:none;
 padding:5px;
}

#links li{
 display:inline;
 font-size: 14pt;
 padding:20px;
}

.sidebar_left {
 float: left;
 width: 180px;
 margin-left:10px;
 position:relative;
 top:-10px;
 text-align: justify;
 line-height:150%;
}

#post {
 float:right;
 width: 680px;
 margin-left:0px;
 margin-right:10px;
 position:relative;
 top:-25px;
 text-align: justify;
 line-height:150%;      
}

#post b {
 font-size:18pt;
 text-decoration:underline;
}

#content {
 float:right;
 width: 680px;
 margin-left:0px;
 margin-right:10px;
 position:relative;
 top:-25px;
 text-align: justify;
 text-indent:25px;
 line-height:150%;
}

#social {
 float:right;
}

#footer {
 width: 890px;
 background:#A7DBD8;
 float:left;
 padding:5px;
 text-align:right;
}

#footer b {
 margin-left:10px;
}

/*End Structure*/

/*images*/
.navimg {
 width:2px;
 height:20px;
}

#tree {
 width:175px;
 height:200px;
 float:right;
 margin-left:20px;
 margin-top:24px;
}

#blackwhite {
 width:200px;
 height:125px;
 float:left;
 margin-right:20px;
 margin-top:10px;       
}

#quickshot {
 width:125px;
 height:100px;
 display: block;
 margin-left: auto;
 margin-right: auto;        
}

I would appreciate it if you can review it for me! BTW, I know it doesn't validate, I am working on that now.

share|improve this question
9  
Could you post the relevant HTML and CSS for us to see? Most people are not going to follow your link. – Jeremy Heiler Jan 27 '11 at 18:36
2  
we have a policy of requiring the important bits of the code to be in the post.. it's fine to post a "see more" link but not showing any code at all is disallowed. I also think generic requests to "review my website!" are a bit too broadly scoped as that encompasses html, css, design.. etc.. – Jeff Atwood Jan 28 '11 at 23:48
@Jeff: Since he specifically mentioned the messiness of the CSS, I it should be added to the question as well (though it does make the question rather crowded). – sepp2k Jan 29 '11 at 0:19
1  
the XML prolog before the doctype makes IE6 trigger the quirks mode – Knu Jan 29 '11 at 3:49

6 Answers

A few quick comments looking through the source:

You did very well in structuring things semantically. I only see a few <br /> or <b> tags. That said, you probably want to include more semantic markup for some things, e.g.,

<div id="post">
    <b>Introduction</b><br />
    <i>Thursday, January 27, 2011</i>
</div>
<br />
<div id="content">

If I were to rework it, I would do:

<div id="post">
 <h1>Introduction</h1>
 <h2>Thursday, blah blah</h2>
</div>
<div id="content">

Then your CSS will style those elements:

div#post h1 { ... }
div#post h2 { ... }

For your images, unless you need a javascript id selector, I'd probably make them a class, rather than unique ID's. It looks like your images will all be styled similarly, so why not group them using a class? Or just override the CSS defaults for the image tag.

Finally, you should probably use a CSS reset. Browsers all use different defaults, so the only sensible thing is to use a reset so that all styling attributes start out the same across all browsers. Eric Meyer is the CSS guru, he has his reset at http://meyerweb.com/eric/tools/css/reset/ (along with more explanation about why to use it).

share|improve this answer
might want to wrap your tags in ``` otherwise things dont show up... – Hailwood Jan 28 '11 at 2:22
Are there such tags as or tags? – systemovich Jan 28 '11 at 8:48
+1 for suggesting <h1> instead of <b>, inter alia. – systemovich Jan 28 '11 at 8:50
I would question the semantics here. <strong> could work as well. <em> should also be used over <i>. I would personally never remove unique identifiers from elements, as the flexibility of being able to style/jQuery a specific element is more efficient than navigating through a list. But a class is a good idea if styles will be reused for many images. – Visionary Software Solutions Jan 29 '11 at 1:30

Your CSS doesn't handle what happens if the reader closes down the window to smaller than your planned size.

Test: resize window to less than 900px.
Results: Window is cut off.

This is a design issue, more than a coding issue.

share|improve this answer
hi can you elaborate on that? i tried firefox + chrome + safari, and when i resize the window, i cant reproduce the problem you are having – Kevin Li Jan 27 '11 at 17:57
I edited the answer. – Hack Saw Jan 27 '11 at 18:01

I didn't look at your site, as I can't afford to click on random links right now.

However, I add overflow: hidden; to the parent element to make sure it wraps its immediate floated child elements. Take my example and try it with and with out the overflow.

HTML:

<div id="page">
    <div id="main">main content</div>
    <div id="side">side content</div>
</div>

CSS:

div#page{
  overflow: hidden;
  background-color: #ccc;
}

div#main{
  float: left;
}

div#side{
  float: right;
}
share|improve this answer

The first line is not needed and I wouldn't do one attribute per line.

share|improve this answer

You have some alt parameters you havent specified too

share|improve this answer
Some details would be worth a +1 :) – palacsint Oct 9 '12 at 18:46

A few semantic issues:

1) Do not use <b> tags.
If you must you can use <strong>or even better <span class="bold"></span> and then style the class in your css.

2) Do not use <i> tags.
If you must you can use <em>or even better <span class="italic"></span> and then style the class in your css.

3) Do not use <br /> tags for spacing.
If you require spacing use margin-bottom, or padding-bottom.

share|improve this answer
1  
Why is <span> better than <i> or <b>? I don't see what semantic value is obtained by replacing those tags. – André Paramés Jan 28 '11 at 3:12
Probably because the W3C says its better to use CSS. I can see the pros and cons.. various levels of bold, different color bold, etc. w3schools.com/tags/tag_phrase_elements.asp – Kevin Jan 28 '11 at 3:17
2  
But that's not a semantic issue. Unless a different class is used, I don't see how "bold" tells anything more about the content (as opposed to telling about the presentation) than <b>. – André Paramés Jan 28 '11 at 3:27
I have suggested that semantic be replaced with good-practice. – systemovich Jan 28 '11 at 11:25
4  
@Kevin You do realise that the W3Schools have absolutely nothing to do with the W3C? – Yi Jiang Jan 28 '11 at 12:23
show 2 more comments

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.