1
\$\begingroup\$

I created a post carousel (it's being used on a WordPress theme initially), but I've adapted it to just HTML/CSS/JS one.

CodePen

var $carousel = $('.post_excerpt_carousel');

$carousel.each(function(){

    var $this = $(this);

    var outer_width = $(window).outerWidth();
    var container_width;

    if (outer_width > 1145) {
        container_width = 1170;
    }

    if (outer_width > 960 && outer_width < 1145) {
        container_width = 960;
    }

    if (outer_width < 960) {
        container_width = parseInt((9/10)*outer_width,10);
    }

    var duration = $this.data('duration');
    var li_number = $this.find('li').length;
    var $ul = $this.find('ul');
    var $li = $ul.find('li');
    $li.removeClass('active');

    if (outer_width < 760){
        $li.css('width', container_width);
        $li.eq(1).addClass('active');
        if ($li.eq(2).hasClass('active')) {
            $li.eq(2).removeClass('active');
        }
    } else if (outer_width > 760){
        $li.eq(1).addClass('active');
        $li.eq(2).addClass('active');
    }

    var list_width = $this.find('li').outerWidth(true);
    var left_offset;

    left_offset = parseInt(list_width - (outer_width - container_width-42)/2, 10);

    $ul.css({'display': 'inline-block', 'width': li_number * $this.find('li').outerWidth(true) + 'px', 'left': -left_offset + 'px'});


    var not_active_no = $this.find('li').not('.first').not('.last').not('.active').length;
    var not_active_width = not_active_no * $this.find('li').outerWidth(true);

    $this.on('click', '.carousel_next', function (e) {
        e.preventDefault();

        if($this.find('li.last').prev().hasClass('active')){
            return;
        } else {
            var $a = $('.active', $this);

            if (!$a.next().hasClass('last') && !$ul.is(':animated')) {
                $a.removeClass('active').next().addClass('active');
            }

            if (!$ul.is(':animated')) {
                $ul.animate({
                    left: parseInt($ul.css('left'), 10) - $ul.find('li').outerWidth(true),
                }, duration);
            }
        }

    });

    $this.on('click', '.carousel_prev', function (e) {
        e.preventDefault();

        if($this.find('li.first').next().hasClass('active')){
            return;
        } else {
            var $a = $('.active', $this);

            if (!$a.prev().hasClass('first') && !$ul.is(':animated')) {
                $a.removeClass('active').prev().addClass('active');
            }

            if (!$ul.is(':animated')) {
                $ul.animate({
                    left: parseInt($ul.css('left'), 10) + $ul.find('li').outerWidth(true),
                }, duration);
            }
        }

    });

});
\$\endgroup\$
7
\$\begingroup\$

Keep your code styling uniform everywhere

  1. Line 3 with your function declaration after $carousel.each( doesn't follow the same styling as used later on in line 50 when declaring the callback function for the click event listener here $this.on('click', '.carousel_prev', function (e) {.
  2. Your if's/else if's/else's don't use the same format. You go from if () { to if (){.

Simplify your if statements

You should change:

if (outer_width > 1145) {
    container_width = 1170;
}
if (outer_width > 960 && outer_width < 1145) {
    container_width = 960;
}
if (outer_width < 960) {
    container_width = parseInt((9/10)*outer_width,10);
}

to use a combination of if(), else if(), and else():

if (outer_width > 1145) {
    container_width = 1170;
} else if (outer_width > 960) {
    container_width = 960;
} else {
    container_width = parseInt((9/10)*outer_width,10);
}

Another example of this is here:

if (outer_width < 760){
    $li.css('width', container_width);
    $li.eq(1).addClass('active');
    if ($li.eq(2).hasClass('active')) {
        $li.eq(2).removeClass('active');
    }
} else if (outer_width > 760){
    $li.eq(1).addClass('active');
    $li.eq(2).addClass('active');
}

It can easily be written as:

if (outer_width < 760) {
    $li.css('width', container_width);
    $li.eq(1).addClass('active');
    if ($li.eq(2).hasClass('active')) {
        $li.eq(2).removeClass('active');
    }
} else {
    $li.eq(1).addClass('active');
    $li.eq(2).addClass('active');
}

This also fixes a problem with a dead spot that happens if outer_width is equal to 760. Unless this is unwanted behavior, which I see no reason why it should be, then the second option is the better way to go. This is an implementation of the TF-Statement or binary condition, where if it's not one, it always has to be the other. Sorry, I've been in math theory recently. :-)

Use simpler and more concise code

Where you define var not_active_no = $this.find('li').not('.first').not('.last').not('.active').length;, you could have easily written a single cleaner and concise $.not(); like so: var not_active_no = $this.find('li').not('.first, .last, .active').length; or you could've just settled with a single CSS call like so: var not_active_no = $this.find('li:not(.first, .last, .active)').length;.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Welcome to Codereview, KingCodeFish. Fine first answer. \$\endgroup\$ – Legato Oct 13 '15 at 4:06
  • \$\begingroup\$ You're very welcome. :-) \$\endgroup\$ – KingCodeFish Oct 13 '15 at 6:40

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.