HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Guild Wars: Knights & Dragons

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
dragonswarsguildknights

Problem

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:


    

Bild

Gallery
Stats
Members
GroupMe
K&D
Apply

Back >>

Knights & Dragons - Wars

    1.
    4.
    7.

    2.
    5.
    8.

    3.
    6.
    9.

Guild info >>
Click here to see all previous wars    

MVP

Duplo - Lv 52

Class: Assassin
Level: 52
Vip level: 2
Ship level: 29
Unlocked exploration areas: 4
Arena rank: (1) 3696
Total contribution: 5624


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;

Solution

HTML:

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

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

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

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

-
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?

-
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:

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

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

Code Snippets

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

html, body {
    height: 100%;
}
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;
    */
}

Context

StackExchange Code Review Q#49460, answer score: 7

Revisions (0)

No revisions yet.