So I have this code. I'm doing something wrong. I want the left arrow to go back (show the previous image) but it isn't working. It shows the next image (like the right arrow). Whatever arrow I click, left or right it shows the next image. I've tried millions of different things and I can't seem to find the problem. Can anyone help me?
I also like the sources to loop. When the last source from the array has been reached, I want to loop back to the first source again (and to the last source when you reach the first).
Btw, I also have a code for the image to change on click. I'm pretty sure it has nothing to do with the problem, but I decided to keep it, just in case it's messing something up.
Thank you :)
HTML:
<div class="container">
<div id="slideshow">
<img alt="slideshow" src="1.jpg" id="imgClickAndChange" onclick="changeImage()"/>
</div>
</div>
JS:
<script>
var imgs = ["2.jpg", "3.jpg", "4.jpg", "5.jpg"];
function changeImage(dir) {
var img = document.getElementById("imgClickAndChange");
img.src = imgs[imgs.indexOf(img.src) + (dir || 1)] || imgs[dir ? imgs.length - 1 : 0];
}
document.onkeydown = function(e) {
e = e || window.event;
if (e.keyCode == '37') {
changeImage(-1) //left <- show Prev image
} else if (e.keyCode == '39') {
changeImage() // right -> show next image
}
}
</script>
<script language="javascript">
var imgs = ["2.jpg", "3.jpg", "4.jpg", "5.jpg"];
function changeImage() {
document.getElementById("imgClickAndChange").src = imgs[0];
imgs.push(imgs.shift())
}
</script>
You were on the right track, but you got a few things wrong:
- You have two variables with the same name.
- You have two functions with the same name.
- If a function expects an argument, you must always send an argument (or
null
), you can not only send -1
for left, and nothing for right. (And in your case, you're actually making it more difficult for yourself by not sending a 1
for right).
|| imgs[dir ? imgs.length - 1 : 0]
, doesn't do what you think it does. You're checking dir
as if it's a boolean. JavaScript can interpret 1
and -1
as booleans (true
and false
respectively), but because you only send along -1
it will not work correctly.
Below is your code with these issues fixed:
JS, HTML:
var imgs = ["http://i.stack.imgur.com/NjC6V.jpg",
"http://i.stack.imgur.com/0eBBQ.gif",
"http://i.stack.imgur.com/uhjjB.png",
"http://i.stack.imgur.com/cNhjf.jpg",
"http://i.stack.imgur.com/Dn175.png"];
document.onkeydown = function(event) {
var e = event || window.event;
if (e.keyCode == '37') { //LEFT
changeImage(-1);
} else if (e.keyCode == '39') { //RIGHT
changeImage(1);
}
}
function changeImage(dir) {
var img = document.getElementById("imgClickAndChange");
img.src = imgs[imgs.indexOf(img.src)+dir] || imgs[(dir==1) ? 0 : imgs.length-1];
}
#imgClickAndChange {
width: 150px;
height: 150px;
}
<div class="container">
<div id="slideshow">
<img id="imgClickAndChange" src="http://i.stack.imgur.com/NjC6V.jpg" alt="" onclick="changeImage(1)" />
</div>
</div>
(
fiddle: http://jsfiddle.net/k3wxhc58/8/)
- In the ternary operator, instead of
(dir==1)
you could just use dir
, since JavaScript translates a 1
to true
. But using (dir==1)
is safer because that will create a real boolean.
- I replaced the images in the
imgs
-array with links to images that actually exist, so that the script works without any errors.
- I added some CSS, this is only for the answer's sake, so that all images have the same size.
If you don't want the images to loop, change the changeImage()
function to this:
function changeImage(dir) {
var img = document.getElementById("imgClickAndChange");
var newSrc = imgs[imgs.indexOf(img.src)+dir];
if (imgs.indexOf(newSrc) != -1) { //only sets new src if it's in the imgs-array (i.e. if it exists)
img.src = newSrc;
}
}
(fiddle: http://jsfiddle.net/k3wxhc58/9/)