6
\$\begingroup\$

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:

<!DOCTYPE html>
<html>
<head>
    <title>Photo Gallery</title>
    <link rel="stylesheet" href="style.css" />
    <script type="text/javascript" src="script.js"></script>        
</head> 
<body>
    <div id="mainContainer">
        <div id="mainImage">
            <img src="img/Flowers_by_Deidameia.jpg" />
        </div>

        <div id="thumbnails">
            <img class="imgThumbnails" src="img/Flowers_by_Deidameia.jpg"/>
            <img class="imgThumbnails" src="img/flower.jpg"/>
            <img class="imgThumbnails" src="img/cone-flowers-preview.jpg" />
            <img class="imgThumbnails" src="img/two_flowers.jpg" />
        </div>
    </div>
    <div id="imgCaption">
    </div>
</body>
</html>

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;
}
Improve this question
\$\endgroup\$
7
\$\begingroup\$

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:
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    (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.

Improve this answer
\$\endgroup\$
6
\$\begingroup\$

This code could use some changes

  • Using window.onload is a bit old skool, consider using this.
  • If you put id="mainImage" on the image instead of on the div, you can change your code to var mainImage = document.getElementById("mainImage");
  • j = (j < 3) ? ++j : 0 ; has the magic constant 3 , which really should point to thumbnailImages.length
  • stillTimer is a global, which is unnecessary. The following code would solve this:
function startTimer(){
  return setInterval(function(){
    j = (j < 3) ? ++j : 0 ;
    mainImage.src = thumbnailImages[j].src;
  }, 10 * 1000);
}   

var stillTimer = startTimer();
Improve this answer
\$\endgroup\$

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.