15
\$\begingroup\$

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

/* 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;
        -moz-border-radius-topleft: 50px;
       -moz-border-radius-topright: 50px;
     -moz-border-radius-bottomleft: 50px;
        border-bottom-right-radius: 50px;
            border-top-left-radius: 50px;
           border-top-right-radius: 50px;
         border-bottom-left-radius: 50px;
}

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

#header {    
     -moz-border-radius-topleft: 50px;
    -moz-border-radius-topright: 50px;
         border-top-left-radius: 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;
        -moz-border-radius-topleft: 50px;
       -moz-border-radius-topright: 50px;
     -moz-border-radius-bottomleft: 50px;
        border-bottom-right-radius: 50px;
            border-top-left-radius: 50px;
           border-top-right-radius: 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;        
}
<?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>

By the way, I know it doesn't validate. I am working on that now.

\$\endgroup\$
  • 10
    \$\begingroup\$ Could you post the relevant HTML and CSS for us to see? Most people are not going to follow your link. \$\endgroup\$ – Jeremy Heiler Jan 27 '11 at 18:36
  • 3
    \$\begingroup\$ 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.. \$\endgroup\$ – Jeff Atwood Jan 28 '11 at 23:48
  • \$\begingroup\$ @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). \$\endgroup\$ – sepp2k Jan 29 '11 at 0:19
  • \$\begingroup\$ The first line is not needed and I wouldn't do one attribute per line. \$\endgroup\$ – user1101 Jan 29 '11 at 3:30
  • 1
    \$\begingroup\$ the XML prolog before the doctype makes IE6 trigger the quirks mode \$\endgroup\$ – Knu Jan 29 '11 at 3:49
17
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ might want to wrap your tags in ``` otherwise things dont show up... \$\endgroup\$ – Hailwood Jan 28 '11 at 2:22
  • \$\begingroup\$ Are there such tags as or tags? \$\endgroup\$ – systemovich Jan 28 '11 at 8:48
  • \$\begingroup\$ 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. \$\endgroup\$ – Visionary Software Solutions Jan 29 '11 at 1:30
  • \$\begingroup\$ @VisionarySoftwareSolutions I think you mistook the first piece of code for an answer, I was using it as an input. OP, as you can see there are lots of good ways of doing it. As you progress, you'll develop your own style. I recommend that you be try to develop a consistent style - eg, if you prefer #post instead of div#post, that's fine, but be consistent. \$\endgroup\$ – Travis Leleu Feb 20 '14 at 0:48
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why is <span> better than <i> or <b>? I don't see what semantic value is obtained by replacing those tags. \$\endgroup\$ – André Paramés Jan 28 '11 at 3:12
  • \$\begingroup\$ 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 \$\endgroup\$ – Kevin Jan 28 '11 at 3:17
  • 4
    \$\begingroup\$ 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>. \$\endgroup\$ – André Paramés Jan 28 '11 at 3:27
  • 7
    \$\begingroup\$ @Kevin You do realise that the W3Schools have absolutely nothing to do with the W3C? \$\endgroup\$ – Yi Jiang Jan 28 '11 at 12:23
  • 1
    \$\begingroup\$ Good call on identifying things that shouldn't be done. The rationale, however, is lacking. The point of <strong> and <em> is not to get bold text and italicized text. You can overwrite the default behavior with CSS. The point is to add semantic meaning so that intelligent queries can be done. \$\endgroup\$ – Visionary Software Solutions Jan 29 '11 at 1:32
2
\$\begingroup\$

A couple notes:

Use HTML5

  1. Instead of <div id="header"> consider using an <header>
  2. Instead of <div id="post"> consider using <article>
  3. Instead of <div class="sidebar_left"> use <aside>. Also, you should NOT use "style specific" classes such as "left", "red", etc because it's very easily overridable in CSS and no-longer relevant. I would also suggest sticking with a convention for CSS that uses dashes instead of underscores. When you look through your JavaScript it will be more legible (underscores mean JS, dashes mean selectors or DOM).
  4. Instead of <div id="footer"> use <footer>
  5. Instead of <div id="content"> use <section> or <div id="main" role="main">. You should use role to provide more context.
  6. Anchors with hashes: I'm not sure why you are using a hash in your anchors (ex: <a href="#">Contact Me</a>). I see developers use it to simply enable a pointer cursor. If this is the case you should instead use:

HTML

<a>Contact Me</a> 

CSS

a { cursor: pointer;}
\$\endgroup\$
  • 1
    \$\begingroup\$ There is no point to add a { cursor: pointer;}. The practice of using <a href="#">Link</a> is common sense to all of us that a url will be inserted later. \$\endgroup\$ – JGallardo Feb 3 '14 at 20:55
  • \$\begingroup\$ Note that this question was posted in 2011, before HTML5 was available. \$\endgroup\$ – 200_success Mar 31 '18 at 14:29
2
\$\begingroup\$

Here is some code golf tips, that can improve performance and code readability:

This:

#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;
}

could be just this:

#wrapper, #footer {
    border-radius: 50px;
    -moz-border-radius: 50px;
}

or if the values differ:

#elem {
    border-radius: 50px 0 25px 10px; /* Top 50px, Right 0, Bottom 25px, Left 10px */
    /* or */
    border-radius: 50px 0; /* Top & Bottom 50px, Right & Left 0 */
}

This rule also applies when you are using margin's or padding's, example:

margin: 0 auto;
margin-top: 30px;

could be:

margin: 30px auto 0;
\$\endgroup\$
2
\$\begingroup\$

Leave out empty CSS rules

In your example, you have these

h2 {}

p {}

You can have one file for development that has those so you can work on them later. But in your production file, leave those out to reduce your file size. It also makes your files more readable when you submit them for review.

Do not use <br> tags for spacing

<br />
<img id="blackwhite" src="images/c3_i7.png" /> Ut venenatis diam nunc...<br />
<br />

If you want to create white space around images. You could do something like

img { margin: 6px; }

You could of course make that margin anything you want. Or you could pass down rules based on how you want them to display at certain cases.

Use class instead of id more often.

In that same element you have this

blackwhite is not something that sounds like it would describe a specific element in a document. But as a matter fact, it looks like you are creating a blog considering how there is a "post." And because you will have many images on this page, you should make this classes.

Use absolute paths

The images/c3_i7.png in <img id="blackwhite" src="images/c3_i7.png" /> would become /images/c3_i7.png which is good on a server because it helps prevent errors if things get moved around.

\$\endgroup\$
1
\$\begingroup\$

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;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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