Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I'm not a css expert so please help me figuring this out. Would you consider this as good css code?

Please point any issues if you believe this is not what you would exactly consider as good code.

                               html { -webkit-text-size-adjust: 70%;  }
  body {  margin:0px; padding:0px;font-family:Arial, Helvetica, sans-serif;  }
 { font-size:14px;}
 #wrap { width:100%; height:100%; background-color:#FFFFFF; padding: 0;}

  #header {
  padding: 10px;

  height:251px;
  }

  #menu { width:100%; height:2.2em; margin-top:1%;  background:#80b2e4; border-      radius:10px; -moz-border-radius:10px; -webkit-moz-border-radius:10px;}
 .top_menu_left {float:left; width:86%;height:35px; margin-left:3%;}
 .top_menu_left li a    { color:#fff !important; }
 .top_menu_left li a:hover  { color:#336699 !important; }

 .top_menu_right {float:right; width:17%;;height:1.2em;}
 li{list-style:none; float:left;  color:#74677E;font-size:70%;font-   weight:bold;font-     family:Arial, Helvetica, sans-serif; text-align:left; padding:0  5px;  -top:9px;  color:#fff;}

.top_menu_left li {padding-right:1%; color:#fff;}
.top_menu_right li {padding-right:5%; color:#fff;}

#searchbar { width:100%; height:auto;margin-top:8%;}

#top_search {width:100%; height:auto;}
#bottom_search {width:89%; height:auto;padding-left:10.5%;font-size:75%;}

.sarch_box { width:10%; height:auto; float:left; margin-left:1%;font-size:75%;}

.box_1 { width:20%; height:auto; float:left;font-size:85%;}
.box_1 input { width:80%; height:1.2em;font-size:85%;}
.box_1 select { width:70%; height:1.5em;font-size:85%;}

.box_2 {width:2%; height:auto; float:left;}

#content { width:87%; height:auto; margin-left:11%; margin-top:0; }
#left_content { width:7%; height:auto; float:left; }
#left_searchbar_title {width:95%;height:2%;padding-top:1%;padding-  bottom:1%;padding-     left:5%;background-color:#A5D959;font-family:Arial, Helvetica,   sans-serif;font-size:75%;}
#left_sidebar_content{width:100%;height:2%;background-color:#EFFFD8;font-  family:Arial,   Helvetica, sans-serif;font-size:75%;}

 #main_content { width:59%; height:auto;float:left;  }
.main_content_header  {margin-left:2%;color:black;font-size:100%;font- family:Arial, Helvetica, sans-serif;font-weight:bold;margin-bottom:5%;}

.content_header {width:70%; height:auto; margin-left:3%;}

 /*.left_1 {margin-left:2%;width:.05em; height:100px;background- image:url(images/verticalbar.gif);background-repeat:repeat-y;float:left;padding-left:2px;    }*/
 .left_1 {margin-left:2%;width:.05em; height:100px;background-image:none;background-   repeat:repeat-y;float:left;padding-left:2px; }
/*.left_6 { width:.05em;height:100px;background-   image:url(images/verticalbar.gif);background-repeat:repeat-y;float:left; }*/
.left_6 { width:.05em;height:100px;background-image:none;background-repeat:repeat-   y;float:left; }

.left_content_middle    {width:95%;float:left;font-size:80%;font-family:Arial, Helvetica, sans-serif; background:white; border:1px solid #c6c6c6;border-radius:10px; webkit-border-radius:10px; -moz-border-radius:10px}

.content_title {padding:2%; text-align:center; color:black; <!--#336699;-->font-  weight:bold;font-size:100%;font-family:Arial, Helvetica, sans-serif; }

.content_line{size:2px; width:95%; background-image:url(images/horizotal-line.gif);}

.left_2 { margin-left:3%; width:21.5%; height:auto; float:left; }
.left_3 { width:23.5%;height:auto;float:left;}
.left_4 { width:23.5%;height:auto;float:left;}
.left_5 { width:23.5%;height:auto;float:left;}


  #right_content { width:20%;height:auto;background-color:white;float:left;margin-   left:1%;padding:10px; border:1px solid #c6c6c6; border-radius:10px; webkit-border-   radius:10px; -moz-border-radius:10px}
 .right_top { width:100%; height:auto; color:white; font-size:100%; color:#336699;}

 .Left_portion { width:45%; height:auto; float:left;padding-left:8%;}

 .right_portion { width:45%; height:auto; float:left;}

 .right_content_title {color:#BB532E;font-weight:bold;font-size:70%;font-family:Arial,  Helvetica, sans-serif;padding-bottom:4%;padding-top:5%; }
 .right_content_body {color:#5E6472;font-size:70%;font-weight:bold;font-family:Arial, Helvetica, sans-serif;padding-bottom:3%;}



  .menu_bar { width:10%; font-family:Arial, Helvetica, sans-serif; color:red; float:left; font-size:50%;}

  .gap {width:35%;}

   #footer   {height:40px;width:100%;margin:auto;text-align:center;text-  align:center;margin-top:2%;background:url('images/footer_bg.png') repeat-x;    line-height:40px; color:white;}
  .footer_menu {
   color: white;
   float: right;
   height: 1.2em;
   -left: 3%;
   width: 70%;
   }
     .footer_menu li {
     color: white;
     padding-right: 1%;
  }
    .footer_menu li a   { color:#fff; text-decoration:none;}
    .footer_menu li a:hover { text-decoration:underline}
    .footer_menu2 {
    background: url("images/footer_bg2.png") repeat-x scroll 0 0 transparent;
    color: grey;
    float: left;
    font-size: 13px;
    margin-top: 21px;
    width: 100%;
    }
   .footer_menu strong  { font-weight:lighter; color:#666}
share|improve this question
 
Whats your markup those goes with this CSS? –  Dan Dec 28 '11 at 22:36
1  
Looks pretty good to me, whole point of using a css file is to reduce redundancy, and you don't seem to have much. Maybe the .Left_i from 3-5, but other than that it looks pretty good. Really depends on what you are trying to get out of it. –  Travis J Dec 28 '11 at 22:37
 
What's the context? It looks like everything is relatively sized so the page will look different when the browser window is a different resolution. Can we see the HTML it's associated with? Also, is there a specific question you have about the css? "Good" and "bad" are kind of arbitrary right now –  tedski Dec 28 '11 at 22:38
 
Others will add more specifics, but as with all code, grouping and commenting CSS will greatly aid in maintainability. –  Ed Schembor Dec 28 '11 at 22:40
2  
There is a lot of redundancy and overspecification in your stylesheet. Too many !important declarations, too many repeated selectors and properties... the one thing that you don't have enough of is indentation. –  BoltClock Dec 28 '11 at 23:18

migrated from stackoverflow.com Dec 28 '11 at 23:20

This question came from our site for professional and enthusiast programmers.

2 Answers

up vote 4 down vote 
html { -webkit-text-size-adjust: 70%;  }
body {  margin:0px; padding:0px;font-family:Arial, Helvetica, sans-serif;  }

There's no need to specify the unit (px) if the value is 0

{ font-size:14px;}

This isn't styling anything

#wrap { width:100%; height:100%; background-color:#FFFFFF; padding: 0;}

Assuming #wrap is a div or other block level element it's unnecesarry to give it a width of 100% as all block elements are 100% wide by default. Padding is also 0 by default in almost all elements.

#header {
padding: 10px;

height:251px;
}

You should never give elements (except images/canvases) a height because if the user increases the font-size the element should adjust to the new size of the content.

#menu { width:100%; height:2.2em; margin-top:1%;  background:#80b2e4; border-      radius:10px; -moz-border-radius:10px; -webkit-moz-border-radius:10px;}

Again, no need to set the width and why would you suddenly use em for the height? It's common to also put the -browser-specific properties before the "real" ones.

.top_menu_left {float:left; width:86%;height:35px; margin-left:3%;}
.top_menu_left li a    { color:#fff !important; }
.top_menu_left li a:hover  { color:#336699 !important; }

Having something like "left" (or right, bold, red, etc) as a class name is not separating the content from the presentation. Also you should try to avoid using !important as it often leads to more use of !important and hard to find bugs.

.top_menu_right {float:right; width:17%;;height:1.2em;}

Again the class name is design related.

li{list-style:none; float:left;  color:#74677E;font-size:70%;font-   weight:bold;font-     family:Arial, Helvetica, sans-serif; text-align:left; padding:0  5px;  -top:9px;  color:#fff;}

Do you really want all your li:s in your entire document to have this styling? There's also syntax errors (like -top).

.top_menu_left li {padding-right:1%; color:#fff;}
.top_menu_right li {padding-right:5%; color:#fff;}

Again the class name - also, this should probably be next to the other styling of the same element.

#searchbar { width:100%; height:auto;margin-top:8%;}

No need to set width or height here as you set them to default values. Also - why would you use an #id here but a .class for almost everything else?

I'll stop quoting code here as the rest contains pretty much the same faults.

A few things I would improve:

  • Don't use design related class or ID-names like left and right.
  • Don't use !important - instead plan your code in a way that doesn't require it.
  • Don't use percentages for everything.
  • Structure your CSS in a way that makes a little more sense. You don't have to keep it all in one file (in fact, you probably shouldn't (but you should only serve one file to the browser though)). This tip would probably require an entire book but a few things to get you started:
    • Start with basic element selectors (like h1, p, a, ul etc) - the generic styling that will be default for everything on your page.
    • Next do your layout - right/left columns and other areas for your "modules".
    • Last style your modules - the #searchbar should have this bg, #recent-comments links should be red, the #footer's ul shouldn't have any bullets etc.

Also, since everything else in HTML uses dash-separation I use that for my class and ID names as well.

share|improve this answer
 
!important is very necessary when doing adaptive (or reactive) design. Dumb example: body article br {display: none} would mean there's no actual break when I use a <br> tag in within an <article> tag. But, lets say that I insert the <br> to cause a break on screen sizes less than 320px @media screen and (max-width: 320px){ body article br {display: block !important;}} without the !important that style will not display properly. !important exists for some very good reasons and should be used when appropriate since there are times when you can't "html around" it. –  tkone Dec 31 '11 at 1:53
1  
Or you could just put that styling after the first. –  powerbuoy Dec 31 '11 at 3:16
 
I still don't see why using !important is "bad" and why it should be avoided. ergo css-tricks.com/when-using-important-is-the-right-choice –  tkone Jan 3 '12 at 15:42
 
Well, in the article you link to it's explained why it's "bad" in the first paragraph. I agree that it can be useful sometimes but very rarely. I've used it to to style .todo elements to always have a red dotted border. But as the name suggests the .todo class is very temporary and never makes it to the live site. –  powerbuoy Jan 4 '12 at 9:02
 
I agree that !important should only be used when absolutely necessary. It's often used in a lazy fashion, which causes hell for developers that come across this later. –  danchet Oct 10 '12 at 17:40
up vote 1 down vote

I don't beleave it is "good" CSS code because:

  • It is inconsistant (white space, multi/single line, etc)
  • There are obvious syntax errors
  • There is a lot of repetition
  • It is mostly percent based sizes which will break in a lot of cases
  • There is rules that will be used by default anyway, hence don't need to be specified
  • There is too much use of the !important keyword
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.