0

I've been working on making a basic little image carousel in jQuery.

Currently at the moment I am stuck on the if else logic inside of my changeImage function.

When the user clicks on the "next" link then the next image in line should fade in. Luckily when I comment out the if else statement I'm able to achieve the images fading out but this is not what I am after. So we know it's a logic issue.

I'm just not sure how to implement the correct syntax with combining conditions within my if else statement and I'm sure this logic could also be much cleaner.

Please review 

function changeImage (newIndex) {
var i = newIndex;
var current = i;

// `if` user clicks on next then slide image "right" 

// something wrong here with my logic..

if ((newIndex === 'next') && i === (current < lengthOfImages - 1)) {
    return current + 1;
}
else {
    return 0;
}

// fadeout
listOfImages.fadeOut(transitionSpeed).
eq(i).fadeIn(transitionSpeed);
}

// click function on the next link
$('.next').on('click',function() {
changeImage('next');
});

Some feed back on how to fix this with a few hints towards a solution would be greatly appreciated.

JSFiddle http://jsfiddle.net/kapena/v82pvq7x/1/

brent_white
  • 1,009
  • 1
  • 13
  • 26
  • i see a string being passed in to `changeImage` where it gets assigned to `i` and `current` . later you treat `current` like a number. i think your `changeImage` function needs tweaking – user2524973 May 27 '15 at 01:28
  • if you want a [CSS-only solution](http://stackoverflow.com/q/30295085/2476755) – royhowie May 27 '15 at 01:47

3 Answers3

1

Return statement will exit the function. Anything after it will NOT run. If you want to actually return the number, you need to do it at the end.

I think you actually want to set current and not to return. Also your logic really does not make any sense. Most people would do the check like this:

current++;
if (current >= lengthOfImages) {
    current = 0;
}
epascarello
  • 204,599
  • 20
  • 195
  • 236
  • @eepascarello thax well all the logic is here . So you are saying to do away with the returns so inside of the `if` we should only see `current = 1;` not a `return` – brent_white May 27 '15 at 01:28
  • well when I try your solution my logic still seems to fail. Back to all images fading away on a single click of the next link – brent_white May 27 '15 at 02:02
1

When 'Next' is clicked, the following is happening:

  1. changeImage is fired, passing in 'next' as its parameter.
  2. Within this function, a variable of i is declared and set as 'next'.
  3. A variable of current is also being set to i, which is currently set to 'next'.
  4. Your if statement checks to see if newIndex(the passed in parameter) is equal to 'next' as well as if i is equal to a boolean of current < lengthOfImages - 1. This is evaluating to a boolean, and i is not a boolean. This is why your function is not firing appropriately.
  5. Your return statements in your conditionals are causing your function to complete, making it so your fadeOut and fadeIn transitions never get a chance to execute.
Richard Kho
  • 5,086
  • 4
  • 21
  • 35
  • thanks I think see what you are saying..the problem is accruing because we need `i` to be `true` and at the moment it is not truthy. To fix this I should remove the `===` from `i === (current < lengthOfImages - 1`operator and use `||` instead? – brent_white May 27 '15 at 01:53
  • `(current < lengthOfImages -1)` will return a boolean. `i` is a number. A number is not a boolean. That second condition for your `if` statement literally checks for `'next' === true/false` – Richard Kho May 27 '15 at 02:00
  • So we **dont** want to return a number and we **do** want to return a truthy value. Dose this mean that I need to do away with `i`? – brent_white May 27 '15 at 02:11
  • Correct! If you're looking for validation that `current` is definitely less than `lengthOfImages - 1`, then it should be: `if (i && (current < lengthOfImages - 1))` – Richard Kho May 27 '15 at 02:28
  • so the condition of our `if` statement should be this `( i && (current < lengthOfImages - 1))` as you stated above correct? – brent_white May 27 '15 at 03:19
  • If your validation check is for current to be one less than the length of images at all times, then yes... – Richard Kho May 27 '15 at 13:31
0

This part

if ((newIndex === 'next') && i === (current < lengthOfImages - 1))

is always false:

i = 'next'
current = 'next'
(current < lengthOfImages - 1) is a boolean

therefore the === is always false, and flow goes to the return clause.

Marco Regueira
  • 1,045
  • 8
  • 17