My custom javascript Carousel is not working properly

Question

I'm trying to implement a carousel with Javascript but I'm having trouble with my navigation buttons.

Prev and Next works fine but the navigation dots as I'm calling are not.

Also, I want to show the title of the images on a span as per the HTML on jsfiddle. You can see my attempt on the javascript file bellow the following comment.

// show the title of the image when hovering the associated dot.

Here is the javascript. The other files can be seem at jsfiddle

$(document).ready(function() {

    var carouselItems = [
        { src: "http://placehold.it/600x300/cccccc/000000", title: "Sample 01" },
        { src: "http://placehold.it/600x300/f45a45/000000", title: "Sample 02" },
        { src: "http://placehold.it/600x300/b78d65/000000", title: "Sample 03" },
        { src: "http://placehold.it/600x300/666aa0/000000", title: "Sample 04" },
        { src: "http://placehold.it/600x300/cccddd/000000", title: "Sample 05" }
    ];

    Carousel = function() {
        // keep track of the current position
        var position = 0;

        // build carousel based on items in the carouselItems array
        $(carouselItems).each(function(index, value){
            var li = $('<li/>');
            li.addClass('carousel-item');
            li.css('width', 100 / carouselItems.length + '%');
            li.appendTo($('#carousel'));

            var img = $('<img/>');
            img.attr('src', value.src);
            img.appendTo(li);

            var liDot = $('<li/>');
            liDot.addClass('carousel-dots-nav-item').html('o');
            liDot.appendTo($('#carousel-dots-nav'));
        });

        // increase width of the carousel
        $('#carousel').css('width', carouselItems.length * 100 + '%');

        // add events to dots
        for (i = 0; i < $('.carousel-dots-nav-item').length; i++) {
            var dot = $('.carousel-dots-nav-item')[i];

            // show the title of the image when hovering the associated dot
            $(dot).hover(function(e){
                $('#title').text(carouselItems[i].title);
            }, function(e){
                $('#title').text('');
            });

            // move to the appropriate slide when a dot is clicked
            $(dot).click(function(e){
                position = i;
                $('#carousel').animate({
                    left: -position * 100 + '%'
                }, 500);
            });
        }

        // add click event to next button
        $("#next").click(function(e){
            e.preventDefault();

            if (position == carouselItems.length - 1) return;

            position++;
            $('#carousel').animate({
                left: -position * 100 + '%'
            }, 500);
        });

        // add click event to previous button
        $("#previous").click(function(e){
            e.preventDefault();

            if (position == 0) return;

            position--;
            $('#carousel').animate({
                left: -position * 100 + '%'
            }, 500);
        });
    };

    var carousel = new Carousel();
});

jsfiddle

You should use `var Carousel = function …`; better style/practice to keep it within scope. — royhowie, Mar 07 '15 at 07:48
@royhowie I prefer `function Carousel() {` and talking about style/practice, `carouselItems` should be passed into it, the function is not a constructor so it should be `createCarousel` (camel case) and not use `new` with it — Ruan Mendes, Mar 07 '15 at 07:48
@royhowie Actually, you should because variables (using function expressions) are hoisted but function declarations are not https://javascriptweblog.wordpress.com/2010/07/06/function-declarations-vs-function-expressions/ — Ruan Mendes, Mar 07 '15 at 07:52
@Vinicius I don't see any attempt at setting the dots when you click previous or next. One question per per post makes it more useful to others. Lastly, code improvements are a different site, http://codereview.stackexchange.com/ — Ruan Mendes, Mar 07 '15 at 07:56
@JuanMendes doesn't [the article](https://javascriptweblog.wordpress.com/2010/07/06/function-declarations-vs-function-expressions/) agree with what I said? "I can see how using Function Declarations can cause confusion but are there any benefits?" and "…there are few (if any) situations where you can’t use a Function Expression assigned to a variable instead" — royhowie, Mar 07 '15 at 07:59
@royhowie I don't think it agrees with what you said. You said beware of hoisting, but function declarations do not get hoisted, only variables. See http://jsfiddle.net/mendesjuan/kwq3ovfc/ — Ruan Mendes, Mar 07 '15 at 08:11
@JuanMendes That didn't throw an error in my browser (Safari), which somewhat demonstrates what I was trying to point out (but didn't really say explicitly enough): how hoisting works inconsistently across browsers. Either way, “*Function declarations* [emphasis mine] and function variables are always moved (‘hoisted’) to the top of their JavaScript scope by the JavaScript interpreter” — royhowie, Mar 07 '15 at 08:24
@royhowie I think I understand our misunderstanding :) yes, function declarations do get moved (hoisted) to the top. I just don't understand what the case is that we should beware of? I said that you should beware with function expression because the var declaration is hoisted but not the assignment of the function, so the variable won't have a value until the actual assignment — Ruan Mendes, Mar 07 '15 at 08:38

score 1 · Answer 1 · edited Jun 20 '20 at 09:12

In this loop:

for (i = 0; i < $('.carousel-dots-nav-item').length; i++) {
    var dot = $('.carousel-dots-nav-item')[i];
    /*...*/

    // move to the appropriate slide when a dot is clicked
    $(dot).click(function(e){
        position = i;
        $('#carousel').animate({
            left: -position * 100 + '%'
        }, 500);
    });
}

You are using i to define the position. But all that is in the click function is not executed inside the loop but when you click on a dot element. So i is equal to $('.carousel-dots-nav-item').length. And you are lucky to have defined it globally.

fiddle

A solution is to store the position of each item in a data-position attribute. As you are using jquery, I used also it:

//
            var liDot = $('<li/>');
            liDot.data("position", index);
            liDot.addClass('carousel-dots-nav-item').html('o');
            liDot.appendTo($('#carousel-dots-nav'));

//
            $(dot).click(function(e){
                var position= $(this).data('position');
                $('#carousel').animate({
                    left: -1*position * 100 + '%'
                }, 500);
            });

Nice, I didn't understand what the OP was asking, I thought they were trying to highlight the current dot; Note that even if there was a `var i`, the behavior would be exactly the same. You should also show the solution here instead of requiring us to figure it out from your link. — Ruan Mendes, Mar 07 '15 at 08:35
I do not have just add the var statement, but I forgot to put the modified version, which is in the fiddle. I edit. — Gaël Barbin, Mar 07 '15 at 09:17

My custom javascript Carousel is not working properly

1 Answers1

fiddle