PDA

View Full Version : Code review - any suggestions?



hurloon
Jan 21st, 2007, 11:42 PM
well, I just made this code when I was messing around, to me it doesn't seem too professional, but I am posting it here open to suggestions on how to make this menu-system more attractive, anyone is free to use it if they choose

click on the 'moose' link to expand the menu.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
<title>...
</title>
<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1" />
<style type="text/css">
ul {
float : left;
text-align : left;
margin : 0;
padding : 0;
list-style : none;
position : relative;
width : 149px;
z-index : 999;
border-width:1px;
border-style:solid;
border-color:steelblue;
}
ul li {
width : 149px;
list-style : none;
margin : 0;
padding : 0;
position : relative;
border-width : 0;
float : left;
}
ul#moose {
display:none;
border-width:0px;
}
ul#moose li {

}
ul li a {
text-indent : 15px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : white;
background-color : steelblue;
padding-top : 3px;
padding-bottom : 3px;
}
ul li a:hover {
text-indent : 15px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : #333333;
background-color : lightblue;
padding-top : 3px;
padding-bottom : 3px;
}
ul#moose li a{
text-indent : 30px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : #333333;
background-color : lightyellow;
padding-top : 3px;
padding-bottom : 3px;
}
ul#moose li a:hover{
text-indent : 30px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : #333333;
background-color : khaki;
padding-top : 3px;
padding-bottom : 3px;
}
</style>



</head>

<body>
<ul>
<li><a href="#">link1</a></li>
<li><a href="#">link1</a></li>
<li><a href="#"onClick="getElementById('moose').style.display='inline'">moose</a></li>
<ul id="moose">
<li><a href="#">link3</a></li>
<li><a href="#">link3</a></li>
<li><a href="#" onClick="getElementById('moose').style.display='none'"style="background-color:burlywood;padding:0px;">^^^^^^^^^^</a></li></ul></li>
<li><a href="#">link1</a></li>
<li><a href="#">link1</a></li>
<li><a href="#">link1</a></li>
</ul>
</body>
</html>

Troy297
Jan 22nd, 2007, 01:13 AM
If you are looking to try and make your code better then you should first try the w3 validator, a great tool for webdesigners everywhere.... but other than that without a live example we can't really suggest visual changes now can we?

http://validator.w3.org/ (use the direct input box at the bottom)

VIPStephan
Jan 22nd, 2007, 01:34 AM
Your CSS is way redundant:



ul {
...
border-width:1px;
border-style:solid;
border-color:steelblue;
/* can be shortened to border: 1px solid steelblue; */
}
ul li {
width : 149px;
list-style : none;
padding : 0;
position : relative; /* only necessary if you specifically wanna use the advantage of positioning (e.g. z-index) */
border-width : 0;
/* not necessary because already specified for ul or simply because it's not necessary*/
float : left; /* needless if links are as wide as navigation and not supposed to be in one line */
}
ul#moose {
display:none;
border-width:0px;
}
ul#moose li {

}
ul li a {
text-indent : 15px;
font-size : 11px;
font-family : "verdana", arial; /* quotes aren't necessary unless you are using a font name that consists of more than one words */
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : white;
background-color : steelblue;
padding-top : 3px;
padding-bottom : 3px;
/* could be shortened to padding: 3px 0;*/
}
ul li a:hover {
text-indent : 15px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
/* not necessary because already specified in rule before */
color : #333333;
background-color : lightblue;
padding-top : 3px;
padding-bottom : 3px;
/* not necessary - same reason as above */
}
ul#moose li a{
text-indent : 30px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : #333333;
background-color : lightyellow;
padding-top : 3px;
padding-bottom : 3px;
/* both bolded sections are not necessary because of above mentioned reasons */
}
ul#moose li a:hover{
text-indent : 30px;
font-size : 11px;
font-family : "verdana", arial;
width : 149px;
font-weight : 500;
display : block;
text-decoration : none;
color : #333333;
background-color : khaki;
padding-top : 3px;
padding-bottom : 3px;
/* all not necessary except of background color */
}


Basically you only need to specify a certain property if you are changing something. If the color stays the same for a link and the same link on mouse over then you don't have to specifiy that. Same applies to all other styles. Only specify if you are changing something.

Also why are you using XHTML transitional? Go for strict if you are not transitioning from older code.