Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to get my first CSS layout reviewed. First of all, the related HTML code is as follows -

<!DOCTYPE html>

<html lang="en">
    <head>
        <meta charset="utf-8" />
        <title> Preferences </title>

        <script src="options.js"></script>
        <link rel="stylesheet" href="options.css" />
    </head>

    <body>
        <h1> Search in New Tab - Preferences </h1>
        <form id="preference-form">
                <label for="search-engine"> Search Engine </label>
                <select id="search-engine">
                    <option id="google" value="google" selected="selected"> Google </option>
                    <option id="bing" value="bing"> Bing </option>
                    <option id="yahoo" value="yahoo"> Yahoo! </option>
                    <option id="duckduckgo" value="duckduckgo"> DuckDuckGo </option>
                    <option id="wikipedia" value="wikipedia"> Wikipedia </option>
                </select>

                <br>

                <label for="focus-search-tab"> Make the search tab active </label>
                <input type="checkbox" value="focus-search-tab" id="focus-search-tab" />
        </form>     
    </body>
</html>

I wrote the following CSS file in order to provide satisfactory presentation to the HTML -

h1
{
    width : 40%;
    background : #eeeeee;
    box-shadow : 0 0 5px 5px #888;
    margin-top : 10%;
    margin-left : 30%;
    margin-right : 30%;
    padding-top : 10px;
    padding-bottom : 10px;
    text-align : center;
    font-family : "Arial", "Century Gothic", "Lucida Sans Unicode";
}

form
{
    width : 30%;
    background : #eeeeee;
    box-shadow: 0 0 5px 5px #888;
    margin-top : 1%;
    margin-left : 35%;
    margin-right : 35%;
    padding-top : 10px;
    padding-bottom : 10px;
}

label
{
    width : 60%;
    display : inline-block;
    text-align : right;
    margin-top : 10px;
    margin-bottom : 10px;
    margin-right : 10px;    
    font-family : "Arial";
}

The resulting HTML document looks like this:

screenshot

Basically, I wanted to avoid the use of float property. I felt that I could accomplish the same by using combinations of inline-block property, padding and margins. So, I used that. Was that the right way? Other than that, I would like to know if I did things right. Also, any improvements that make my CSS code better are welcome.

share|improve this question
    
Get rid of the <br> –  Shmiddty Jan 4 '13 at 19:28
add comment

2 Answers

I'm no CSS expert, but two things I would change are:

  • remove unnecessary spaces (indenting and in ' : ').
  • condense explicit margin-top, margin-left etc (and padding):

    h1{
    width:40%;
    background:#eee;
    box-shadow:0 0 5px 5px #888;
    margin:10% 30% 0 30%;
    padding:10px 0 10px;
    text-align:center;
    font-family:"Arial", "Century Gothic", "Lucida Sans Unicode";
    }
    
    form{
    width:30%;
    background:#eee;
    box-shadow: 0 0 5px 5px #888;
    margin:1% 35% 35%;
    padding:10px 0 10px;
    }
    
    label{
    width:60%;
    display:inline-block;
    text-align:right;
    margin:10px 10px 10px;
    font-family:"Arial";
    }
    
share|improve this answer
5  
+1 for condensing items. I have to disagree with the indenting of lines. I find it easier to read and find the different tags within the file with the white space. –  Jeff Vanzella Oct 23 '12 at 20:39
    
@JeffVanzella, yes it is true. Perhaps indent with a single TAB rather than many spaces (possibly the OP did that). –  William Morris Oct 23 '12 at 20:57
    
That would work, but in VS tab is replaced by spaces ;) –  Jeff Vanzella Oct 23 '12 at 21:22
    
@WilliamMorris yes. I am used to a tab indent rather than several spaces. Thanks for the condensation tip, though; it looks better, and saves a lot of lines. I will stick with indentation, though. –  Ghost Oct 24 '12 at 6:51
    
However, I was hoping for a review of the code that I have written for CSS - like if my usage of inline-block + margin + padding is better than taking the headache of using float, to achieve the same result. –  Ghost Oct 24 '12 at 6:53
show 1 more comment

I think the no fundamental difference using the "float:left;" and "display:inline-block;". In the first case, we have to clear the output before inserting the new elements that are not on the floated blocks. In the second case we make of inline element inline-block.

Here's another variant:

<!DOCTYPE html>

<html lang="en">
<head>
    <meta charset="utf-8">
    <title> Preferences </title>
    <link rel="stylesheet" href="options.css">
</head>

<body>
    <h1> Search in New Tab - Preferences </h1>
    <br>
    <form id="preference-form">
        <label> Search Engine
            <select>
                <option id="google" value="google" selected> Google </option>
                <option id="bing" value="bing"> Bing </option>
                <option id="yahoo" value="yahoo"> Yahoo! </option>
                <option id="duckduckgo" value="duckduckgo"> DuckDuckGo </option>
                <option id="wikipedia" value="wikipedia"> Wikipedia </option>
            </select>
        </label>

        <label> Make the search tab active 
            <input type="checkbox" value="focus-search-tab" id="focus-search-tab">
        </label>
    </form>     
    <script src="options.js"></script>
</body>
</html>​

Styles:

body { text-align: center; }
h1, form {
  display: inline-block;
  background: #eee;
  box-shadow: 0 0 5px 5px #888;
  font-family: Arial,Helvetica,Garuda,sans-serif;
}

h1 {
  margin: 10% auto 1%;
  padding: 10px;
  text-align: center;
}

form {
  margin: 1% auto;
  padding: 10px;
}

label {
  display: block;
  margin: 10px;
}​
share|improve this answer
1  
There are fundamental differences. Just look at this floated thing and imagine the amount of trickery necessary to push all things into place: jsfiddle.net/JAyfJ –  ANeves Nov 14 '12 at 16:44
    
You want to make the transfer of the <label> to a new line? Where, then, <br>? Red border can fix plus one line in css: jsfiddle.net/JAyfJ/1 –  Ademaro Nov 15 '12 at 12:04
    
1. The <br> element "br elements must be used only for line breaks that are actually part of the content, as in poems or addresses"., and so should not be used to send the label to the new line. 2. USing overflow: hidden; in the container is curious, I had not thought of that and cannot find any flaws in it. –  ANeves Nov 15 '12 at 12:50
    
What about the element <br> - I agree. For more semantics then it is logical to make the form elements out of <p> tag or wrap them in a separate <div>. Because they are not part of paragraph. –  Ademaro Nov 15 '12 at 14:09
add comment

Your Answer

 
discard

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

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