6
\$\begingroup\$

I am working on a (HTML/CSS) website to improve my skills but I am not sure if I am doing it the right way, because I have a lot of floats and sizes. I also need help with some CSS things:

What I have:

i.imgur.com/r3VGbWy.png

What I am Shooting for WishList Picture

The red dimensions in the image are the dimensions I've tried to give the objects and which I am not sure if it is the correct way of doing it. The black words are the things I would like to change in the future, but I need this code reviewed first.

All my code: INDEX.HTML

<div id="wrapper">
   <div id="mainContent">
      <div id="newsHolder">
         <div id="item1">
            <img src="img/item1.jpg">
         </div>
         <div id="item2">
            <img src="img/item2.jpg">
         </div>
         <div id="item5">
            <img src="img/item3.jpg">
         </div>
         <div id="item3">
            <img src="img/item4.jpg">
         </div>
         <div id="item4">
            <img src="img/item5.jpg">
         </div>
      </div>
   </div>
   <div id="sidebar-right">
      <p>sidebar</p>
   </div>
   <div id="newsList">
      <p> Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book.</p>
   </div>
</div>
<!--<div id="footer"></div>-->

STYLE.CSS

@import url(reset.css);

body {
background:#f1f1f1;
}

#fullHeader {
float:left;
min-width:100%;
height:90px;
}

#header {
min-width:100%;
height:60px;
background:#B52B42;
}

#inner-header {
width:1180px;
height:60px;
margin:0 auto;
}

#home {
float: left;
width: 140px;
}

#menubar{
float: left;
width: 1180px;  
}

#wrapper {
width:1180px;
margin:0 auto;
-moz-box-shadow:0 0 5px #888;
-webkit-box-shadow:0 0 5px #888;
box-shadow:0 0 5px #888;
}

#mainContent {
width:100%;
float:left;
background-color:#FFF;
}

#newsHolder {
float:left;
width:840px;
height:493px;
}

#item1 {
float:left;
width:477px;
height:320px;
margin-bottom:5px;
}

#item2 {
float:right;
width:358px;
height:244px;
margin-bottom:5px;
}

#item3 {
float:left;
width:236px;
height:167px;
margin-right:5px;
}

#item4 {
float:left;
width:236px;
height:167px;
}

#item5 {
float:right;
width:358px;
height:244px;
}

#item1 img,#item2 img,#item3 img,#item4 img,#item5 img {
width:100%;
height:100%;
}

#sidebar-right {
float:right;
width:340px;
background:#FFF;
}

#newsList {
float:left;
width: 840px;
background:#FFF;
}

#footer {
float:left;
min-width:100%;
height:70px;
background:#006;
}

I did not post the CSS code of the navigation menu because it is already working correctly.

I would be very happy if anyone can help me make my HTML/CSS cleaner

\$\endgroup\$
3
  • \$\begingroup\$ Code review is not for "fixing HTML/CSS layout to look correctly". Code Review is essentially about having working and correct code, then rewriting parts of that code so that you still get the same result but in a better way. Your question is therefore off-topic because it's about fixing a problem. \$\endgroup\$ Commented Dec 16, 2013 at 15:22
  • \$\begingroup\$ Keep your CSS as DRY as Possible. you are repeating your width several times. item 3 & 4 differ only by a margin-right 2 & 5 by a margin-bottom you could add a class to 2 & 5 and then add another class for the margin, I hope that makes sense. then if you want another row you can just repeat. you shouldn't use ID's for those items, because then you can't use that style again. \$\endgroup\$
    – Malachi
    Commented Dec 16, 2013 at 16:08
  • \$\begingroup\$ you should provide all the html, I don't know what is going on with your Headers, but the CSS for them looks wrong. you should apply the width to the outermost element and then 100% the elements inside that you want to be the same width. \$\endgroup\$
    – Malachi
    Commented Dec 16, 2013 at 16:09

1 Answer 1

7
\$\begingroup\$

instead of using ID's you should be using classes. like this

<div id="wrapper">
   <div id="mainContent">
      <div id="newsHolder">
         <div class="item1">
            <img src="img/item1.jpg">
         </div>
         <div class="item2">
            <img src="img/item2.jpg">
         </div>
         <div class="item5">
            <img src="img/item3.jpg">
         </div>
         <div class="item3">
            <img src="img/item4.jpg">
         </div>
         <div class="item4">
            <img src="img/item5.jpg">
         </div>
      </div>
   </div>
   <div id="sidebar-right">
      <p>sidebar</p>
   </div>
   <div id="newsList">
      <p> Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book.</p>
   </div>
</div>
<!--<div id="footer"></div>-->

then you could change it a little bit more because you have the same exact CSS for 3 & 4 so I would change those to something like this.

         <div class="smallLeft rtMargin"> <!-- Generic Class Names -->
            <img src="img/item4.jpg">
         </div>
         <div class="smallLeft">
            <img src="img/item5.jpg">
         </div>
#smallLeft {
    float:left;
    width:236px;
    height:167px;
}

    #rtMargin {
    margin-right:5px;
}

same concept for items 2 & 5, and remember that CSS executes top to bottom and will overwrite itself if you change an element with two different selectors in different CSS documents or even on the same document it will overwrite the style, the last one will prevail (in that instance)


Something from my Comment

you should provide all the html, I don't know what is going on with your Headers, but the CSS for them looks wrong. you should apply the width to the outermost element and then 100% the elements inside that you want to be the same width. – Malachi 1 hour ago

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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