1
\$\begingroup\$

I have created a video/article slider. It works fine, but I have a feeling it's not quite a standard solution.

I would appreciate it if someone could review my code, so that if there are mistakes, I could learn from them.

<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js" type="text/javascript"></script>
<script> 

var move = function slide() {
    $('#container').animate({'marginLeft' : '-=707px'}, 500);
}

 //setInterval(function(){slide()},3000000);

</script> 


<script>

  window.addEventListener("load", function(){

      //slide 1, slide 2
      setTimeout(function(){       
      var video0 = document.createElement('video');
      video0.src = 'images/video/trx.mp4';
      video0.id = 'video0';

      video0.autoplay = true;
      video0.controls = true;

      document.getElementById("vidcont_0").appendChild(video0) 


       slide2();
       },30000);


 });

       //slide 3
       var slide2 = function(){     
       move(); 

       setTimeout(function(){       
       slide3();    
    }, 30000);

}

        //slide 4
        var slide3 = function(){            
        move();  

        setTimeout(function(){ 
        var video1 = document.createElement('video');
        video1.src = 'images/video/trx.mp4';
        video1.id = 'video1';

        video1.autoplay = true;
        video1.controls = true;

        document.getElementById("vidcont_1").appendChild(video1);          
        slide4();

         }, 30000);

}

        //slide 5
        var slide4 = function(){        
        move();        
        setTimeout(function(){      
        slide5();   

       }, 30000);

}


        //slide 6
        function slide5(){          
        move();  

        setTimeout(function(){

        video2 = document.createElement('video');
        video2.src = 'images/video/trx.mp4';
        video2.id = 'video2';

        video2.autoplay = true;
        video2.controls = true;

       document.getElementById("vidcont_2").appendChild(video2);        
       //window.open("http://server.info-spot.net", "_self");    
       slide6();       

      }, 30000);

}

        //slide 7
        var slide6 = function(){    
        move();        
        setTimeout(function(){      

       move();

       }, 30000);

}






</script>
 <style type="text/css">

 body {}
 #wrapper {width:707px; height:820px;overflow:;}
 #container {width:5000px;height:820px;}

#video0 {width:707px; height:820px;float:left;}
#video1 {width:707px;height:820px;float:left;background:##F93;}
#video2 {width:707px;height:820px;float:left;background:#093;}
#video3 {width:707px;height:820px;float:left;background:#F0F;}
#article0 {float:left;width:707px;height:820px;background:#999;}
#article1 {float:left;width:707px;height:820px;background:#f00;}
#article2 {float:left;width:707px;height:820px;background:#06F;}
#article3 {float:left;width:707px;height:820px;background:#06F;}

</style>


</head>
<body>



<div id="wrapper">
<div id="container">

<div id="article0">Article 0</div>

<div id="vidcont_0"></div> 

<div id="article1">Article 1</div>

<div id="vidcont_1"></div> 

<div id="article2">Article 2</div>

<div id="vidcont_2"></div> 

<div id="article3">Article 3</div>





</div><!----end container---> 
</div>   <!----end wrapper--->
</body>
</html>
\$\endgroup\$
1
\$\begingroup\$

I would consider having the css be in an external css file and javascript in an external js file since there is quite a bit of non-HTML code, but that is just my own preference.

I would get rid of extra whitespace and a lot of rework can be done with indentation to make the code more readable.

You can also group variable assignments together:

var video1 = document.createElement('video');
video1.src = 'images/video/trx.mp4';
video1.id = 'video1';
video1.autoplay = true;
video1.controls = true;

And group methods together:

document.getElementById("vidcont_2").appendChild(video2);        
slide6();

I would consolidate <script> tags that are right next to each other.

Remove old code that is commented out if you're not planning on using it.


Reformatted:

<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js" type="text/javascript">

var move = function slide() {
    $('#container').animate({'marginLeft' : '-=707px'}, 500);
}

window.addEventListener("load", function(){

    //slide 1, slide 2
    setTimeout(function(){
        var video0 = document.createElement('video');
        video0.src = 'images/video/trx.mp4';
        video0.id = 'video0';
        video0.autoplay = true;
        video0.controls = true;

        document.getElementById("vidcont_0").appendChild(video0);
        slide2();

    },30000);
});

//slide 3
var slide2 = function(){     

    move(); 

    setTimeout(function(){       
        slide3();    
     }, 30000);
}

//slide 4
var slide3 = function(){            
    move();  

    setTimeout(function(){ 
        var video1 = document.createElement('video');
        video1.src = 'images/video/trx.mp4';
        video1.id = 'video1';
        video1.autoplay = true;
        video1.controls = true;

        document.getElementById("vidcont_1").appendChild(video1);          
        slide4();

         }, 30000);
    }

    //slide 5
    var slide4 = function(){        
    move();        
    setTimeout(function(){      
    slide5();   

    }, 30000);
}

//slide 6
function slide5(){          
     move();  

     setTimeout(function(){
         video2 = document.createElement('video');
         video2.src = 'images/video/trx.mp4';
         video2.id = 'video2';
         video2.autoplay = true;
         video2.controls = true;

         document.getElementById("vidcont_2").appendChild(video2);        
         slide6();       

      }, 30000);
}

//slide 7
var slide6 = function(){    
    move();        
    setTimeout(function(){      
        move();
    }, 30000);
}

</script>

<style type="text/css">
body {}
#wrapper {width:707px; height:820px;overflow:;}
#container {width:5000px;height:820px;}
#video0 {width:707px; height:820px;float:left;}
#video1 {width:707px;height:820px;float:left;background:##F93;}
#video2 {width:707px;height:820px;float:left;background:#093;}
#video3 {width:707px;height:820px;float:left;background:#F0F;}
#article0 {float:left;width:707px;height:820px;background:#999;}
#article1 {float:left;width:707px;height:820px;background:#f00;}
#article2 {float:left;width:707px;height:820px;background:#06F;}
#article3 {float:left;width:707px;height:820px;background:#06F;}
</style>

</head>

<body>
<div id="wrapper">
    <div id="container">
        <div id="article0">Article 0</div>
        <div id="vidcont_0"></div> 
        <div id="article1">Article 1</div>
        <div id="vidcont_1"></div> 
        <div id="article2">Article 2</div>
        <div id="vidcont_2"></div> 
       <div id="article3">Article 3</div>
    </div><!----end container---> 
</div>   <!----end wrapper--->
</body>

</html>

Additional Note: I would also improve the comments to be more descriptive and helpful.

  • <!----end container---> is unnecessary if you have proper indentation
  • //slide 5 right before var slide4 is confusing
  • If you keep HTML, CSS, and JavaScript in the same file I would comment each section as such.
\$\endgroup\$
1
  • \$\begingroup\$ Thank you really much for the suggestions. I have nicely separated the code formats in separate files. Now it is more clear and nice to have only HTML to work with in the index file. \$\endgroup\$ – Edmond Tamas Jun 28 '13 at 21:16

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.