7
\$\begingroup\$

I've been using CSS and HTML for a while and I'm trying to start to use better methods of coding. I've tried my best not to hack anything. Could anyone let me know how I'm doing at the moment in terms of using things which are better practice?

Please note - I've taken some image names and comments out to protect the privacy of this project, as it's still a work in progress. I hope this won't affect any reviews!

*,p
{
    margin:0;
    padding:0;
}

html
{
    background:#517c24 url(images/backgroundHTML.jpg) no-repeat top center;
}

body
{
    font-family:"HelveticaNeue", "Helvetica Neue", Helvetica, Arial, sans-serif;
    background:url(images/backgroundBody.png) repeat-x top left;
}

.clear
{
    clear:both;
}

#container
{
    background:#fff url(images/header.jpg) no-repeat top left;
    width:960px;
    overflow:auto;
    -webkit-box-shadow:0 0 5px 2px rgba(0, 0, 0, 0.3);
    -moz-box-shadow:0 0 5px 2px rgba(0, 0, 0, 0.3);
    box-shadow:0 0 5px 2px rgba(0, 0, 0, 0.3);
    margin:0 auto;
}

.login-search
{
    float:right;
    text-align:right;
    width:210px;
    height:70px;
    font-size:13px;
    color:#6e7074;
    margin:30px 10px;
}

.login-search a
{
    text-decoration:none;
    color:#6e7074;
}

.login-search p
{
    margin:5px 0;
}

input
{
    border:1px solid #ccc;
    outline:none;
    -moz-border-radius:2px;
    -webkit-border-radius:2px;
    border-radius:2px;
    color:#777;
    margin-bottom:10px;
    padding:6px 4px;
}

.float
{
    float:right;
    width:356px;
    height:335px;
    clear:both;
    margin:30px 20px 0;
}

.inner-wrap
{
    margin-top:300px;
}

.navigation
{
    width:230px;
    height:300px;
    background-color:#e5e5e5;
    float:left;
    margin:0 30px;
    padding:5px;
}

.design-your-own
{
    height:110px;
    width:250px;
    max-width:250px;
    background-color:#596f86;
    float:left;
}

.se
{
    height:70px;
    width:250px;
    max-width:250px;
    border-top:1px solid #000;
    border-bottom:1px solid #000;
    float:left;
    margin:10px 0;
}

.brand-contain
{
    width:600px;
    max-width:600px;
    float:left;
    margin-top:10px;
    position:relative;
}

.many-brands
{
    background-color:#ab1322;
    height:110px;
    width:250px;
    max-width:250px;
    float:left;
}

.brand-contain img
{
    margin-top:-80px;
}
<!DOCTYPE>
<html xmlns="http://www.w3.org/1999/xhtml">
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
        <title></title>
        <link rel="stylesheet" type="text/css" href="stylesheet.css" />
    </head>

    <!--- start body --->
    <body>

        <!--- start container --->
        <div id="container">

            <!--- login and search box --->
            <div class="login-search">
            <p><a href="login.cfm">Login</a> / <a href="register.cfm"> Register </a></p>
            <p><input type="text" name="search" id="search" style="width: 100px;" /></p>
            </div>
            <!------>


            <!--- image --->
            <div class="float">
                <img src="images/img.png" width="356" height="335" />
            </div>
            <!------>

            <div class="inner-wrap">

                <!--- left column, so navigation and contact link --->
                <div class="left-column">
                    <!--- navigation bar --->
                    <div class="navigation">
                        <ul>
                            <li>Home</li>
                            <li>Standard</li>
                            <li>Special</li>
                            <li>Design your own</li>
                            <li>About us</li>
                            <li>Contact us</li>
                            <li>Order help</li>
                            <li>Shipping service</li>
                            <li>Terms &amp; Conditions</li>
                            <li>Shipping Information</li>
                            <li>Testimonials</li>
                        </ul>
                    </div>
                    <!------>
                </div>
                <!------>


                <!--- 3 main buttons on homepage --->
                <div class="middle-content">

                    <!--- design --->
                    <a href="design-your-own.cfm">
                        <div class="design-your-own">
                            Design your own >
                        </div>
                    </a>
                    <!------>

                    <!--- comment --->
                    <a href="">
                        <div class="se">
                            Text Text! >
                        </div>
                    </a>
                    <!------>

                    <!--- many brands! --->
                    <a href="design-your-own.cfm" >
                    <div class="brand-contain">
                            <div class="many-brands">
                                Many brands >

                            </div>
                        <img src="images/selectManyBrands.png" border="0" />
                      </div>
                    </a>
                    <!------>

                </div>
                <!------>

            <div class="clear"></div>

            <p>More text ></p>

        </div>
        <!------>

    </body>
    <!------>


    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.6.1/jquery.min.js"></script>
    <script src="https://cdn.jsdelivr.net/jquery.shadow-animation/1.11.0/jquery.animate-shadow-min.js"></script>
    <script>
        $('#search')
                .focusin(function() {$('#search').animate({
                    width: '200',
                    boxShadow:  '0 0 3px rgba(0,0,0,.4)'
                    }, 300)
                .focusout(function() {$('#search').animate({
                    width: '100',
                    boxShadow:  '0 0 0 rgba(0,0,0,0)'
                    }, 300)
            });
        });
    </script>

</html>

\$\endgroup\$
1
\$\begingroup\$

Depending on the size of your project, module/API reliance I would load jquery in the [head] section through google's CDN. This way if you need to use any Google API (charts,analytics) down the line the script already is loaded. Loader

Script loading

<script src="https://www.google.com/jsapi"></script>
<script>
    google.load("jquery", "1.7.1");
    google.load("jqueryui", "1.8.4");

    // somewhere down the line I decide I want to use 
    // google charts
    google.load("visualization", "1");
</script>

Some 3rd party slider scripts function better when the script is loaded in the [head] section, I find. Some common problems, screen flicker, page jumps.

Htaccess

I would move [http-equiv] to .htaccess file

Use the .htaccess and remove these lines to avoid edge case issues. - html5boilerplate

<IfModule mod_headers.c>
  Header set X-UA-Compatible "IE=Edge,chrome=1"

  <FilesMatch "\.(js|css|gif|png|jpe?g|pdf|xml|oga|ogg|m4a|ogv|mp4|m4v|webm|svg|svgz|eot|ttf|otf|woff|ico|webp|appcache|manifest|htc|crx|oex|xpi|safariextz|vcf)$" >
    Header unset X-UA-Compatible
  </FilesMatch>
</IfModule>

Resets

Use resets like twitter-bootstrap, html5boilerplate

*{} is considered bad practice You could get away with it in this context

*{
  background-color: white !important;
  color: black !important;
}

.float

This seems a little strange to me. I wouldn't add a height/width on this class. maybe you should re-name this class to something more specific. Float to me represents a floating element, not a specific element such as you wrapping your img tag inside.

.float_rt{
   float:right;
   clear:right;
} 

Typography

It does not look like you have styling specific to typography. Consider using rules such as line-height, font, letter-spacing on the body/relevant tags

body{
   font: normal 64.5% sans-serif;
   line-height: 1.5;
   color:#585858;
}
h1,h2,h3,h4,h5,h6{
   font: bold 100% sans-serif;
   margin-bottom:18px;
   letter-spacing:-1px;
}
p{
   margin-bottom:18px;
   line-height:1.6;
   font-size:1.3em;
} 

Clears

consider this common clearfix example

.clear{
   display: block;
}
.clear:after{
   content: ' ';
   visibility: hidden;
   height:0;
   clear:both;
}

Or just use the after pseudo on each element you want cleared.

.naviagtion:after{
    content: ' ';
   visibility: hidden;
   height:0;
   clear:both;
   display:block;
}

Navigation menu

You should really wrap your li content in anchor tags to help search engines follow them.

\$\endgroup\$
  • \$\begingroup\$ Thanks Philip, all comments have been taken into account! I also never knew about the :after selector! It makes my code look cleaner because I dont have to have <div class="clear"> everywhere! \$\endgroup\$ – Joshua M Mar 28 '12 at 8:45
1
\$\begingroup\$

This code seem nice to me although they are certain things that I would do differently.

First I don't understand why you've put you jquery script at the end. I always put my script at the beginning in the head so that my script are loaded before my content. If there is javascript that needs to be loaded after the DOM is fully loaded, for example your focusin, focusout methods, I put in the jquery ready function like that :

$(function () {
     // Your code here
}

which is short for

$.ready(function () {
     // Your code here
}

That way, your JS is not far away at the bottom at the page. Maybe you have a reason and I make a mistake.

Second you've created a float class so why don't you use it directly on the img ?

<img class="float" ...... />

instead of

<div class="float"><img .... /></div>

A part from that, your code is very readable and also commented so I don't see many other remarks.

\$\endgroup\$
  • \$\begingroup\$ Thanks! :-) It's just a practice of mine to put jQuery at the end of the page. I don't think it affects page loading times too much, and yeah - it was at the bottom like that so it doesn't fire before the DOM loads, not that it ever would due to it being based on Event Handlers. \$\endgroup\$ – Joshua M Mar 27 '12 at 14:43
  • 4
    \$\begingroup\$ It is considered a good practise to put script elements at the bottom of the page, concerning page load time; because browser will block parallel downloads while downloading a script element. \$\endgroup\$ – Dennis Mar 27 '12 at 15:08
  • \$\begingroup\$ Ok my mistake. I learned something :) \$\endgroup\$ – Patrick Browne Mar 29 '12 at 9:15

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.