patternjavascriptMinor
Guild Wars: Knights & Dragons
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:
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;
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
That said, what about your table with the ID
-
Use descriptive names. The filenames
CSS:
-
Why do you select both
This is, what you should have:
I removed the units behind
The declaration
-
You should declare your basic font rules in the body rule above as well.
After this you could specifiy some basic rulesets for stuff like headings and maybe a few variations for smaller text (meta data, etc.).
-
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.