HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

A script for a photo gallery/slideshow

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptgalleryslideshowforphoto

Problem

I'm a beginner in JavaScript. I wrote this code so the main image get set on whatever thumbnail image the user clicks.

And if the user didn't click on any image the main photo get changed every 10 seconds.

Here's my code (I feel like I'm doing some unnecessary work here in the timers but I can't figure it out):

HTML:


    Photo Gallery
    
            
 

    
        
            
        

        
            
            
            
            
        
    
    
    


JavaScript:

window.onload = function(){
    var mainImage = document.getElementById("mainImage").getElementsByTagName("img")[0],
        thumbnailImages = document.getElementById("thumbnails").getElementsByTagName("img"),
        j = 0;  

    var startTimer = function(){
        stillTimer = setInterval(function(){
                j = (j < 3) ? ++j : 0 ;
                mainImage.src = thumbnailImages[j].src;

            }, 10000);
    }   

    startTimer();

    for(i = 0 ; i < thumbnailImages.length ; i++){
        thumbnailImages[i].addEventListener("click", function(evt){     
            clearInterval(stillTimer);
            mainImage.src = evt.target.src;                 
            ++j; 
            startTimer();
        });
    };
}


CSS:

body {  
    margin: auto;
    width: 800px;
    height: 800px;
}

#mainContainer {    
    width: 760px;
    height: 800px;
}

#mainImage {
    margin: auto;      
    height: 560px;  
}

#mainImage img {
    margin: 10px;  
    width: 740px;
    height: 540px; 
}

#thumbnails {
    padding-top: 10px;
    height: 205px;
}

.imgThumbnails {
    margin-left: 6px;
    height: 186px;
    width: 180px;
    display: inline;
    hover: 
}

.imgThumbnails:hover {
    cursor: pointer;
}

Solution

I'm not a JS guy, so I focus on your HTML and CSS.

HTML

  • You're using the HTML5 doctype, so you can safely ommit the type-attribute on the script-tag



  • Add the viewport meta tag to the head-area to ensure correct viewport behavior on mobile devices:


`
(Note: I left out the self-closing
/ for the meta tag as well)

  • Avoid using ID's excessively; Things like caption and even a gallery itself are likely to occur multiple times on the same page – Classes are more appropriate there



CSS:

  • You have an error in the .imgThumbnails -rule, where you use a hover`-property; There is no such property!



  • Side note: There is also no semi-colon. Although one can ommit the last semi-colon in a list of property-declarations, I highly encourage anyone not to do this. It just leads to unnecessary trouble



  • You're using a lot of fixed widths and heights. This is very restrictive and should be avoided.



Before I get deeper in to making suggestions on how to improve your code further, it would be great to have some kind of demo. This doesn't need to be a working example of your gallery, you can use placeholders instead of images.

Context

StackExchange Code Review Q#14103, answer score: 7

Revisions (0)

No revisions yet.