7
\$\begingroup\$

I have coded a webpage and it looks exactly how I want, but I think there could be improvements, possible as unnecessary floats, etc...

Could anybody please review my CSS code? Its not difficult or vague, I guess.

DEMO: http://jsfiddle.net/dqC8t/2364/

CSS:

body{
background: url("http://i.imgur.com/cQhlsYZ.png") repeat-x;
}

h1{
font-size: 18px;
font-weight: bold;
color: #444;
}

h2{
font-size: 16px;
font-weight: bold;
color: #444;
}

#wrapper{
width: 980px;
margin: 0 auto;
}

/*************START HEAD CONTENT*************/
#header{
width: 100%;
float: left;
}

#headerLogo{
float: left;
margin-top: 20px;
margin-left:  20px;
margin-right:  70px;
}

#menu  {
font-family: 'ProximaNova-Bold';
font-size: 16px;
margin-top:40px;
}

#menu ul {
    list-style-type: none;
}
#menu li {
  display: inline-block;
  width: 150px;
  border-bottom-style: solid;
  border-bottom-width: 4px;
  margin: -5px ;
  padding: 0 15px 0 0 ;
}

.item-1 {
  border-bottom-color: #0099CC;
}
.item-2 {
  border-bottom-color: #FF4444;
}
.item-3 {
  border-bottom-color: #669900;
}
.item-4 {
  border-bottom-color: #FFBB33;
}

.item a {
  text-decoration: none;
}

#headContent{
float: left;
margin-top: 20px
}

.hnItem{
width: 242px;
height: 184px;
float: left;
margin-right: 4px;
}

.hnItem  img {
display: block;
}

#hnItemLast{
width: 242px;
height: 184px;
float: left;
}

#hnItemLast img{
display: block;
}

.hnTextContainer{
height: 40px;
padding: 10px 15px;
font-family: 'ProximaNova-Regular';
font-size: 14px;
line-height: 21px;
color: #c8cbcb;
background-image: linear-gradient(#262828,#1c1e1e);
}
/*************END HEAD CONTENT***************/

/*************START MAIN CONTENT*************/
#mainContent{
width: 980px;
height: 800px;
float: left;
margin-top: 20px;
background-color: #FFF;
}

/*************START NEWS LIST CONTENT********/
#nlContainer{
width:660px;
float:left;
font-family: 'ProximaNova-Regular';
}

#nlContainer p{ 
font-size:14px;
}

#nlContainer a{ 
text-decoration: none;
}

#nlHeader{
float: left;
width:100%;
margin-top: 15px;
margin-bottom: 5px;
margin-left: 20px;
}

.nlItem{
width: 100%;
float: left;
margin-top: 10px;
margin-left: 20px;
}

.nlImageContainer{
width: 200px;
height: 100px;
float: left;
padding: 3px;
border: 1px solid #e3e3e3;
background-color: #efefef;
}

.nlTextContainer{
float:left;
margin-top: 10px;
margin-left: 20px;
}

.rmLink  a{
font-size: 14px;
color: #069;
}

.rmLink span{
font-size: 10px;
color: #c73f20;
}
/*************END NEWS LIST CONTENT**********/

/*************START SIDEBAR CONTENT**********/
#sidebar{
width: 300px;
height: 400px;
float:left;
margin-right: 20px;
}

#scBanner{
margin-top: -10px;
}

HTML:

  <div id="wrapper">
        <div id="header">
            <div id="headerLogo">
                <a href="index.html"><img alt="myDr logo" height="50" width="134" src="http://i.imgur.com/w4FYag7.png"></a>
            </div>
            <div id="menu">
                <ul>
                    <li class="item item-1"><a href="">GEZONDHEID A-Z</a></li>
                    <li class="item item-2"><a href="">MEDICIJNEN</a></li>
                    <li class="item item-3"><a href="">GEZOND LEVEN</a></li>
                    <li class="item item-4"><a href="">NEWS &amp; EXPERTS</a></li>
                </ul>
            </div>
        </div>
        <div id="headContent">
            <div class="hnItem">
                <a href="#"></a>
                <div class="hnImageContainer"><img alt="#" height="124" src="http://i.imgur.com/w4FYag7.png" width="242"></div>
                <div class="hnTextContainer">Lorem ipsum dolor sit amet, consectetur adipisicing elit</div>
            </div>
            <div class="hnItem">
                <a href="#"></a><div class="hnImageContainer"><img alt="#" height="124" src="http://i.imgur.com/w4FYag7.png" width="242"></div>
                <div class="hnTextContainer">Lorem ipsum dolor sit amet, consectetur adipisicing elit</div>
            </div>
            <div class="hnItem">
                <a href="#"></a><div class="hnImageContainer"><img alt="#" height="124" src="http://i.imgur.com/w4FYag7.png" width="242"></div>
                <div class="hnTextContainer">Lorem ipsum dolor sit amet, consectetur adipisicing elit</div>
            </div>
            <div id="hnItemLast">
                <a href="#"></a><div id="hnImageContainer"><img alt="#" height="124" src="http://i.imgur.com/w4FYag7.png" width="242"></div>
                <div class="hnTextContainer">Lorem ipsum dolor sit amet, consectetur adipisicing elit</div>
            </div>
        </div>
        <div id="mainContent">
            <div id="nlContainer">
                <div id="nlHeader">
                    <h1>Laatste nieuws</h1>
                </div>
                <div class="nlItem">
                    <div class="nlImageContainer">
                        <a href="#"><img alt="#" height="100" src="http://i.imgur.com/w4FYag7.png" width="200"></a>
                    </div>
                    <div class="nlTextContainer">
                        <h2><a href="#">Lorem ipsum dolor sit amet consectetur</a></h2>
                        <p>Lorem ipsum dolor sit amet consectetur dolor sit<br>amet consectetur amet</p>
                        <p class="rmLink"><a href="#">Lees meer<span>&gt;&gt;</span></a></p>
                    </div>
                </div>
                <div class="nlItem">
                    <div class="nlImageContainer">
                        <a href="#"><img alt="#" height="100" src="http://i.imgur.com/w4FYag7.png" width="200"></a>
                    </div>
                    <div class="nlTextContainer">
                        <h2><a href="#">Lorem ipsum dolor sit amet consectetur</a></h2>
                        <p>Lorem ipsum dolor sit amet consectetur dolor sit<br>amet consectetur amet</p>
                        <p class="rmLink"><a href="#">Lees meer<span>&gt;&gt;</span></a></p>
                    </div>
                </div>
                <div class="nlItem">
                    <div class="nlImageContainer">
                        <a href="#"><img alt="#" height="100" src="http://i.imgur.com/w4FYag7.png" width="200"></a>
                    </div>
                    <div class="nlTextContainer">
                        <h2><a href="#">Lorem ipsum dolor sit amet consectetur</a></h2>
                        <p>Lorem ipsum dolor sit amet consectetur dolor sit<br>amet consectetur amet</p>
                        <p class="rmLink"><a href="#">Lees meer<span>&gt;&gt;</span></a></p>
                    </div>
                </div>
                <div class="nlItem">
                    <div class="nlImageContainer">
                        <a href="#"><img alt="#" height="100" src="http://i.imgur.com/w4FYag7.png" width="200"></a>
                    </div>
                    <div class="nlTextContainer">
                        <h2><a href="#">Lorem ipsum dolor sit amet consectetur</a></h2>
                        <p>Lorem ipsum dolor sit amet consectetur dolor sit<br>amet consectetur amet</p>
                        <p class="rmLink"><a href="#">Lees meer<span>&gt;&gt;</span></a></p>
                    </div>
                </div>
            </div>
            <div id="sidebar">
                <div id="scBanner">
                    <a href="#"><img alt="#" height="140" src="http://i.imgur.com/w4FYag7.png" width="300"></a>
                </div>
            </div>
        </div>
    </div>
\$\endgroup\$
5
\$\begingroup\$

HTML:

  • For prototyping, you should include the # for links to actually trigger link behavior in browsers: <a href="#">Link</a>
  • Instead of placing <span>&gt;&gt;</span> in your HTML, you should use…
  • If you page is not dynamically generated, you may consider using the CSS properties width and height instead of the HTML attributes

CSS:

  • …the :after pseudo element:

    .rmLink:after {
        /* puts a space and two `&gt;` after rmLink */
        content: "\00A0" "\003E" "\003E";
    }
    
  • If you're not using some kind of CSS reset, you don't need to set font-weight: bold; on headings; All browsers should have User Agent Styles for them.

  • As already suggested, you should atleast provide sans-serif in your font-family declaration as a fallback, especially if you're not using web fonts (which you should consider)
  • When you define hex-based color values, keep in mind you can write #fff instead of #ffffff and so on; There are several occurences in your code where you could change that

Note:

You have a lot of similar margin declarations which can be simplified and stripped down. Think about where you rather should use padding on a parent instead of margin on several child elements.

Check out nlItem and nlTextContainer. You're repeating yourself.

\$\endgroup\$
  • 1
    \$\begingroup\$ Height/width attributes on images should not be replaced with their CSS equivalents. These attributes are not in danger of being deprecated. \$\endgroup\$ – cimmanon Jan 27 '14 at 20:22
  • \$\begingroup\$ @cimmanon You're right. I'm not talking about deprecation. It's just a maintainance nightmare when instead you could have them in your CSS. \$\endgroup\$ – kleinfreund Jan 27 '14 at 20:23
  • 1
    \$\begingroup\$ How is placing it in the CSS any less of a maintenance nightmare? The OP's code looks like it is potentially dynamically generated, meaning the dimensions of the images could be dynamically generated as well. If this is the case, shoving it into the CSS would be the maintenance nightmare since it would have to be regenerated whenever the content is changed. \$\endgroup\$ – cimmanon Jan 27 '14 at 20:26
  • \$\begingroup\$ @cimmanon Although it seems pretty unlikely to me, that this page is generated in such a manner, I agree with you. If this is the case, leaving the attributes where they are is the right thing to do. Thank you. Updated my answer. \$\endgroup\$ – kleinfreund Jan 27 '14 at 20:39
  • \$\begingroup\$ This page is not generated yet, but thats my goal. So you both are right :) \$\endgroup\$ – user3104933 Jan 27 '14 at 23:16
3
\$\begingroup\$

Font:

font-family: 'ProximaNova-Regular';

You need a fallback font (or two). If this one doesn't exist then it needs to find another appropriate font. Usually I will put serif, san-serif or monospace last. For example:

font-family: 'ProximaNova-Regular, Arial, sans-serif';

width: 100% and margin-left is not doing what you think it is doing. Using the content box sizing model your items are 20px outside the parent to the right. Instead put the parent as padding-left: 20px and no margin-left on the item.

 #nlContainer
 {
     width:660px;
     // ...
     padding-left: 20px;
 }

 .nlItem
 {
     width:100%;
     //...
     margin-left: 0;
 } 
\$\endgroup\$
  • \$\begingroup\$ Do I have to change the width of nlContainer? I am also not sure if the floats of the #header #headContent and #mainContent are needed? \$\endgroup\$ – user3104933 Jan 22 '14 at 13:02
  • \$\begingroup\$ @user3104933 The width of nlContainer doesn't have to change, just its padding. \$\endgroup\$ – James Khoury Jan 23 '14 at 2:11
3
\$\begingroup\$

You have in the image an inline style

<img alt="#" height="100" src="http://i.imgur.com/w4FYag7.png" width="200">

And all the images will have the same size. I would set that in the container class (since you have already that in the CSS:

.hnItem img {
    display: block;
    width: 242px;
    height: 124px;
}
\$\endgroup\$
  • \$\begingroup\$ Just to be precise: These are not CSS inline styles. These are HTML attributes. ;-) \$\endgroup\$ – kleinfreund Feb 1 '14 at 17:44
  • \$\begingroup\$ @kleinfreund Yes, you are right. !! \$\endgroup\$ – vals Feb 1 '14 at 19:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.