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 want to improve this code to make it look more professional, be loaded quicker and make it cross-browser compatible. Do note that it is not a finished site. I want to know the best practice for doing certain things before I proceed any further.

HTML:

<!DOCTYPE html>
<html>
<head>
<title></title>    
<link rel="stylesheet" type="text/css" href="css.css"></script>


</head>

<body>

<div id="menu">
<div id="img">Bild</div>

<div id="gallery">Gallery</div>
<div id="stats">Stats</div>
<div id="members">Members</div>
<div id="groupMe">GroupMe</div>
<div id="knd">K&D</div>
<div id="apply">Apply</div>
</div>

<div id="guildinfo">
<span class="info">Back >></span>

</div>
<div id="wars">Knights & Dragons - Wars

<table id="warslist">

<tr>
    <td>1.</td>
    <td>4.</td>
    <td>7.</td>
</tr>

<tr>
    <td>2.</td>
    <td>5.</td>
    <td>8.</td>
</tr>

<tr>
    <td>3.</td>
    <td>6.</td>
    <td>9.</td>
</tr>
</table>
<span class="info">Guild info >></span>
<span id="previous"><a href="previouswars.html">Click here to see all previous wars</a>    </span>
</div>

<div id="content">



</div>

<div id="banner"><b>MVP</b></div>
<div id="content2">
<div id="imgHolder"></div>
<div class="fontstyling center">Duplo - Lv 52</div>
<div id="spanHolder">
<span>Class: Assassin</span><br>
<span>Level: 52</span><br>
<span>Vip level: 2</span><br>
<span>Ship level: 29</span><br>
<span>Unlocked exploration areas: 4</span><br>
<span id="arenaRank">Arena rank: (<span id="yellowColor">1</span>) 3696</span><br>
<span>Total contribution: 5624</span>
</div>
</div>
<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>
<script src="javascript.js" type="text/javascript"></script>
</body>
</html>

CSS:

html, body { 
        margin: 0px; 
        padding: 0px; 
        width: 100%;
        height: 100%;
        background: url(img/background.jpg) no-repeat fixed;
        background-size: cover;
}

a { 
        text-decoration: none;
        color: yellow;
}    

#menu {
        width: 316px;
        height: 100px;
        margin-top: 20px;
        margin-left: 50px;
}

#img { 
        background: url(img/icon.png); 
        background-size: cover; 
        width: 100px;
        height: 100px;
        float: left;
} 

.menuDivs { 
        width: 70px;
        height: 30px;
        color: yellow;
        background: url(img/brown.jpg);
        font-family: "Arial";
        font-size: 14px;
        text-align: center;
        float: right;
        margin: 20px 2px -18px 0px;
        vertical-align: bottom;
        line-height: 30px;
        border-radius: 5px;
}

#wars { 
        width: 450px;
        height: 170px;
        border: 1px solid black;
        border-radius: 10px;
        background: url(img/brown.jpg);
        top: 20px;
        left: 380px;
        position: absolute;
        font-family: "Tahoma";
        font-size: 18px;
        color: yellow;
        text-align: center;
        line-height: 30px;
        opacity: 0;
}

#warslist { 
        width: 400px;
        height: 100px;
        color: white;
        margin: auto;
        font-size: 13px;
        text-align: left;
}

.info {
        position: absolute;
        top: 0px;
        right: 15px;
        font-size: 11px;
        color: white;

}

#previous { 
        position: absolute;
        bottom: 0px; 
        right: 15px;
        font-size: 11px;
}

#guildinfo {
        width: 450px;
        height: 170px;
        border: 1px solid black;
        border-radius: 10px;
        background: url(img/brown.jpg);
        top: 20px;
        left: 380px;
        position: absolute;
        opacity: 0;
        z-index: -1;
        line-height: 30px;
}

#content {   /* main content*/
        width: 700px;
        padding: 50px;
        min-height: 100%;
        background: rgba(0, 0, 0, 0.7);
        margin: 100px auto auto 50px;
        color: white;
        font-family: "Tahoma";
        font-size: 15px;
        border-radius: 9px;
        word-wrap: break-word;
}

#content2 {    /*secondary content (right)*/
        width: 380px;
        min-height: 80%;
        background: rgba(0, 0, 0, 0.7);
        top: 380px;
        right: 88px;
        position: absolute;
        border-radius: 15px;
}

#banner {
        width: 480px;
        height: 150px;
        background: url(img/banner.png);
        background-size: cover;
        top: 320px;
        right: 40px;
        position: absolute;
        z-index: 1;
        text-align: center; 
        line-height: 70px;
        font-size: 20px;
        color: black;
        font-family: "Tahoma";
} 

#imgHolder { 
        width: 270px;
        height: 300px;
        border: 2px solid black;
        margin: 50px auto 5px auto;
        background: url(img/mvp.png) no-repeat -50px;
        background-size: cover;
}

.fontstyling { 
        font-family: "Tahoma";
        font-size: 15px;
        color: white;
}

.center {
        text-align: center;
        margin-bottom: 20px;
}

.marginLeft { 
        margin: auto auto auto 80px;
}    

#yellowColor {
        color: yellow;
}

JavaScript:

$(document).ready(function() {

    $(".info").click(function() {
        if($("#guildinfo").css('z-index') === '-1'){
            $("#guildinfo").css('z-index', '1');
            $("#guildinfo").animate({
                opacity:'1'
            }); //end of animate
        }
        else{
            $("#guildinfo").animate({
                opacity:'0'
            });

            setTimeout(function() {
                $("#guildinfo").css('z-index', '-1');
            }, 500);
        }
    }); //end of info function

    $("#knd").click(function() {
        if($("#wars").css('opacity') === '0') {            
            $("#wars").animate({
                opacity:'1'
            }); //end of animate
        }

        else {
            $("#wars").animate({
                opacity:'0'
            });  //end of animate
        }
    });    //end of KND click function

    $("#gallery, #stats, #members, #groupMe, #apply, #knd").addClass("menuDivs");
    $("#spanHolder").find("span").addClass("marginLeft fontstyling");
    $("#arenaRank").find("span").removeClass("marginLeft");
}); //end of doc ready function
share|improve this question
3  
Please don't make modifications to the supplied code based on the reviewer's suggestions, as it invalidates their answers. –  cimmanon May 11 at 13:55
2  
Could you please describe what this code does? It will help with having a better title, which will make this question more attractive. –  Jamal May 11 at 14:05
    
I accidently did it before I read it. I will make a new question soon that is more descriptive. –  Duplo W May 11 at 14:37
    
@DuploW: You could just clarify this question and ask a new question later if you'd like review of updated code. –  Jamal May 11 at 14:41
    
editing in the first code now so it doesn't make the answers invalid. –  Duplo W May 11 at 14:43

3 Answers 3

up vote 7 down vote accepted

HTML:

  1. HTML5 allows omitting the type attribute from link, script and style tags. You can safely remove it from your link tag.

  2. You should move jQuery to the bottom of your documents. Otherwise it'll be loaded before your document gets rendered.

  3. You use a lot of ID's where you shouldn't. I generally advice avoiding ID's whereever it's possible, because working with classes is easier in terms of CSS specificity.

    Overwriting an ID rule with a class is hard. Also ID's should only be used when you can definitely say there will only be one of these elements on this page.

  4. You rarely indent your HTML code. Do it. It allows you to see the structure you're building and leaves less space for mistakes like not closing certain tags.

  5. The content inside your ID spanholder (again, this should be a class) looks like tabular data. Thus you could use a table for it.

    That said, what about your table with the ID warslist. Should this be a table? Why?

  6. Use descriptive names. The filenames css.css and javascript.js are pretty redundant, because the extension already tells you what to expect from this file.

CSS:

  1. Why do you select both html and body in your first CSS rule? That only makes sense for your declaration of height: 100%;

    This is, what you should have:

    body { 
        margin: 0;
        background: url(img/background.jpg) no-repeat fixed;
        background-size: cover;
    }
    
    html, body {
        height: 100%;
    }
    

    I removed the units behind 0, you don't need it for zero values. I also removed the padding declaration, because body doesn't have a padding you would reset in all User Agent Styles of modern browsers.

    The declaration width: 100%; is also not necessary, because both elements are block level elements. Block elements automatically take up the available space.

  2. You should declare your basic font rules in the body rule above as well.

    body {
        /* Using shorthand instead of... */
        font: 14px/1.5 Tahoma, Arial, sans-serif;
    
        /* ...this:
        font-size: 14px;
        line-height: 1.5;
        font-family: Tahoma, Arial, sans-serif;
        */
    }
    

    After this you could specifiy some basic rulesets for stuff like headings and maybe a few variations for smaller text (meta data, etc.).

share|improve this answer
    
1. Does this cause any compability issues in older browsers? I'm afraid that if I remove it it will not work. –  Duplo W May 11 at 11:57
    
1. Does this cause any compability issues in older browsers? I'm afraid that if I remove it it will not work. 2. I thought that was only for the javascript document, which I have at the bottom, but I will change it. 3. I'm using ID's to be able to easily identify things when working with Jquery, don't know a lot about finding a certain element other than using .find() after $("#id/.class"), maybe you know a few tricks? 4. I will try to do that more often. 5. I could, or could a list do the trick as well? Within warslist I tried to do it with a list and not a table but the list was flowing... –  Duplo W May 11 at 12:03
    
outside of the div, I wanted it to be 3 columns with 3 list items in each column but I could not figure out how to do that so I decided just to use a table instead. Should I use a list instead and how would I get it to function the way I want it to? 6. What should I name it after? Since it's the css for the entire site I don't really know what else to name it. Or should I use multiple external style sheets for different parts of the website? –  Duplo W May 11 at 12:05
1  
@DuploW You can post a follow-up question if you improve your code with the tips you get in your review here. Take your time, try some things out and if you're ready, feel free to ask for new reviews. –  kleinfreund May 11 at 12:13
1  
@DuploW Don't edit your question with new code here. Post a new question. Editing the post with updated code would mean invalidating already made answers. ;) –  kleinfreund May 11 at 12:19

A lot has already been said, but allow me to add a few more remarks.

CSS:

  • Avoid using classes like 'fontStyling' and 'center'. Classes should relate to the content, not to the styling. I believe the styles from 'fontstyling' should be applied to your body. And that 'center' class, what if you decide tomorrow that you want to align it left? Are you going to have a class centerwith a text-align: left or are you going to change it to 'left' and edit both your html and your css? Always try to keep content and styling strictly separated.
  • Definitely use some sort of reset or normalize css. It will make your site look consistent across browsers, and avoids having to reset thing manually (like margin: 0 on your body). And it takes care of the styles you are bound to forget to reset.
  • I see a lot absolute positioning going on. This can be fine in some cases, but it makes it very hard to change things, is a pain to debug cross browser and is bound to cause problems on small screens when you go responsive. Always try to keep your elements in the document flow, and only use absolute positioning as a last resort!
  • the same goes for all those fixed dimesions. They can be fine and usefull, but they can be a pain when you add content, or a different browser renders your font a few pixels higher. Let the browser do its work and let the content determine the size of your elements. It will make your site scale a lot better. And when you realy have to, try to work with min- or max- width or height. That way you can control things while leaving space for the content to alter the dimensions.
  • There are a lot of background images in there. Could be fine, but hard to tell without seeing them. Note that a lot can be achieved with pure css (gradients, filters, font icons). Also consider working with sprites. I know most users have a fast connection nowadays, but that's no excuses for using a lot af bandwith. We all want a faster internet, don't we?

HTML:

  • the & is a reserved character inside html. You should replace it with it's entity &amp;
  • try to use semantic tags, not just divs and spans.
    • A menu is a nav which is a ul of a's.
    • That table, since it contains 1,2,3,4... that should be a hint. This should be an ol split into 3 columns
    • ...
  • using background images is fine in most cases, as they usually are just there for styling. In the case of the #imgHolder however, I suppose that contains some guild logo or something. That would be content imo, and therefore require an actual img. I always follow the rule: If a user prints the page, would he want the image on there? If true, use an img, else a background image.
  • As already said by others, go easy on the id's. And not each element needs an id or a class. You can also target stuff with selectors like #menu > div in stead of adding .menuDiv to each menu item.

JS:

  • You are adding and removing a bunch of css classes at the bottom of your css. Why would you want to do that? Just do that in your HTML already. That way it is visible on page load, you do not need a reflow, and it is a lot easier to maintain.
  • I think you may want to have a look at the jQuery fadeToggle function. It would make those click handlers a whole lot easier. And no more need to play with z-indexes and opacity, jQuery does it all for you out-of-the-box.

Addition:
As promised, I set up a small fiddle to demonstrate what my code would look like: http://jsfiddle.net/jXy4E/

Note that this is not meant to be a visual replica of the OP code. Just a quick rewrite with the remarks I made taken into account:

  • No more styling related classes in the HTML (except for the .clearfix, but that is unfortunately currently the best solution that I know of)
  • Activated the jsFiddle 'normalize css' option
  • Only one fixed position, and as little fixed dimensions as possible. Should be a lot easier to adapt or make responsive.
  • Tried to use more semantic html(5) elements.
  • Demonstrated some slightly more complex css selectors
  • No more adding of classes in the JS
  • A much more flexible solution for the click handlers (button 'members' and 'K&D' should work), with the use of fadeToggle and :visible

I hope it is al clear, but feel free to ask if you want me to explain anything further.

share|improve this answer
    
Some great pointers man thanks. I have changed a lot from the original code already so it looks somewhat better. I have a few questions though: 1. What do you mean by "use a reset or normalize CSS"? 2. What are sprites? 3. How can I split lists into 3 columns? I have tried but I don't understand how. 4. I tried the fadeToggle but it didn't seem to work the way it should have. It's hard to explain when you can't really see what I'm trying to achieve with it. –  Duplo W May 14 at 8:28
    
I managed to split the list into 3 columns, but not with a IE compatible solution however. The other solutions require SO much code that it doesn't seem efficient at all.. –  Duplo W May 14 at 8:50
    
1. necolas.github.io/normalize.css 2. css-tricks.com/css-sprites and for 3 and 4 I'll set up a fidde, hang on there –  PeterVR May 14 at 19:42
    
I'll have a look at those and hanging on for the fiddle –  Duplo W May 15 at 6:17
    
I finally found the time to set up a fiddle, added it in my answer. –  PeterVR May 15 at 19:02

You can start by:

  • Standardize indentation. Use 4 spaces, each child element/new scope goes one indent.
  • Limit newlines to at most 3. Don't leave too much blank lines.
  • Chain if possible. Usually jQuery functions return the same object. In this case, we apply css and animate to the same #guildinfo

    $("#guildinfo")
      .css('z-index', '1')
      .animate({
        opacity:'1'
      });
    
  • Cache values when they are used more than once. This is possible as well.

    var guildInfo = $("#guildinfo")
    guildInfo.css('z-index',1);
    guildInfo.animate({opacity:1});
    
share|improve this answer

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.