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\$
0

1 Answer 1

Reset to default
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;.

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Codereview, KingCodeFish. Fine first answer. \$\endgroup\$ Commented Oct 13, 2015 at 4:06
  • \$\begingroup\$ You're very welcome. :-) \$\endgroup\$ Commented Oct 13, 2015 at 6:40

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.