1

I'm learning javascript and I'm having trouble figuring out why my script is not working. I'm guessing its because the imageIn and imageOut functions don't have access to the counter variable. How would I go about fixing this? Both imageIn and imageOut have errors in my error console 'undefined'.

<style type="text/css">

ul {
    list-style-type:none;
}

</style>

<body>

<div id="slideShow">

    <ul>
        <li>
            <img src="stockboat.png" alt="Steam Boat" id="boat" />
        </li>   
    </ul>

</div>

<script type="text/javascript" src="getElementsByClassName.js"></script>
<script type="text/javascript">

    var image = document.getElementsByTagName('img');

    for (i = 0, ii = image.length; i < ii; i++)  {
        image[i].style.opacity = "0.5";
        image[i].addEventListener('mouseover', imageIn, 'false');
        image[i].addEventListener('mouseout', imageOut, 'false');   
    }

    function imageIn() {
        image[i].style.opacity = "1";   
    }

    function imageOut() {
        image[i].style.opacity = "0.5";
    }
</script>

</body>
</html>
Marc Casavant
  • 297
  • 2
  • 11
  • 2
    `imageIn` and `imageOut` have access to `i` but at the moment these functions are executed, `i` will have a value equal to `image.length`, but `image[image.length]` does not exist. – Felix Kling Apr 21 '12 at 20:52
  • Also note that `addEventListener` is not available in IE8 and below. – Felix Kling Apr 21 '12 at 21:03

5 Answers5

0

I think you're right about it not recognizing your iteration variable, you'll have to do:

Use reference object:

function imageIn() {
    this.style.opacity = "1";         // use "this" instead of image array
}

function imageOut() {
    this.style.opacity = "0.5";        // use "this" instead of image array
}

-or-

Closure Approach:

image[i].addEventListener('mouseover'
                         , (function(obj){return function(){imageIn(obj)};})(image[i])
                         , 'false');
image[i].addEventListener('mouseout'
                         , (function(obj){return function(){imageOut(obj)};})(image[i])
                         , 'false');

Need to change function definition:

function imageIn(obj) {          // added parameters "obj"
    obj.style.opacity = "1";   
}

This method allows you to reference the loop variable, which is nice to have as an example for when using setTimeOut in a loop - so you can reuse code later :)

Community
  • 1
  • 1
vol7ron
  • 40,809
  • 21
  • 119
  • 172
  • 1
    That won't work... at the moment `imageIn` and `imageOut` will be executed, `i` will have the value of `image.length`. Typical *closure in a loop* problem. – Felix Kling Apr 21 '12 at 20:53
  • 1
    Ah... you mean using `this` inside `imageIn` and `imageOut`. Yes, that would work. – Felix Kling Apr 21 '12 at 20:58
  • @FelixKling: yes, but still a closure should work (edited). Also, `imageIn` and `imageOut` need to take a parameter `i` – vol7ron Apr 21 '12 at 20:59
  • Using `this` inside the functions would be the best solution, since `image` is a live collection of nodes, and it could get "out of sync" with the event handlers. I would go with that solution, then you don't even have to change how the functions are bound. – Felix Kling Apr 21 '12 at 21:03
  • @thorn: on my phone and can't efficiently. Felix: edited answer so that's not a problem – vol7ron Apr 21 '12 at 21:03
  • var image = document.getElementsByTagName('img'); for (i = 0, ii = image.length; i < ii; i++) { image[i].style.opacity = "0.5"; image[i].addEventListener('mouseover', imageIn, 'false'); image[i].addEventListener('mouseout', imageOut, 'false'); } function imageIn() { this.style.opacity = "1"; } function imageOut() { this.style.opacity = "0.5"; } – Marc Casavant Apr 21 '12 at 21:05
  • @Felix, I agree, `this` would be less confusing compared to the closure (that's why I originally suggested it in that comment that I deleted on accident). The closure is good for reference, though. – vol7ron Apr 21 '12 at 21:07
  • Does anyone know a good site to get scripts I can learn from? Or is it more effective to learn by doing? – Marc Casavant Apr 21 '12 at 21:11
  • @MarcCasavant: my answer is its always more effective to learn by doing. But that's why good tutorials teach how to do something useful. I have a few bookmarks I'll try to find by reputable authors that teach you the right way (mind you I'm on my phone right now, so it'll be a while) – vol7ron Apr 21 '12 at 21:13
  • @vol7ron Thank you! :) I've been bootcamping JS for the last few days, I did a course by Kevin Yank on learnable.com. It really helped me get a grasp on how code is parsed and event listeners and other features, the DOM. However I still have trouble putting all those things together to create a functioning script. – Marc Casavant Apr 21 '12 at 21:18
  • Your issue above is about scoping, especially regarding `this`. Variables for the most part are either in the local function scope, or they're global (part of the `window` object). That is, in open code `var i=0` is the same thing as `window.i=0`. The `this` keyword can be tricky (especially cross-browser) in how its implemented. – vol7ron Apr 21 '12 at 21:23
  • Yeah, I hear internet explorer is just as bad for JS as it is CSS and everything else. – Marc Casavant Apr 21 '12 at 21:29
  • MS is a weird ship. People complain they aren't standards compliant, but in many cases they are and the other major browsers were not (still IE5.5/6 was a nightmare for us programmers). For JS, a lot of times MS had to build solutions before anything was adopted into ECMA, so they were ahead of the curve and in an industry where standardization is key, it sometimes hurts public image to be innovative. – vol7ron Apr 21 '12 at 21:51
  • @MarcCasavant: MDN's [reference](https://developer.mozilla.org/en/JavaScript/Reference) and [guide](https://developer.mozilla.org/en/JavaScript/Guide) are great resources. Just watch out for anything Mozilla specific (in the reference). – Felix Kling Apr 21 '12 at 22:29
  • Thanks guys for the help and resources! vol7ron did you get a chance to find any of those tutorial? :D My brain is hungry for knowledge! – Marc Casavant Apr 22 '12 at 00:55
  • Also thanks to helpful people like you guys, my girlfriend is highly amused with my javascript animation and keeps claiming that the image I animated is moving! SHE'S AMAZED! LOL – Marc Casavant Apr 22 '12 at 00:58
0

You may try jsfiddle and put the 2 functions imagineIn and imageOut before the code that use them.

hornetbzz
  • 9,188
  • 5
  • 36
  • 53
0

There could be errors in your <script type="text/javascript" src="getElementsByClassName.js"></script> which is causing the rest of your script to stop executing.

In addition to the answers mentioned here I would offer some other advice:

  1. Define your counter variables: var i = 0, ii = image.length. Otherwise you might end up using some global i and ii variables that are already set....
  2. Use an inspector like Chrome Developer Tools to find issues in your JS code.
  3. Add and remove CSS classes rather than changing the elements styles. This will allow you to make multiple style changes with little effort.
  4. Learn to make use of Event Delegation in your code, it will help you in the long run especially when you want to start making use of dynamic content.

Good luck with your learning.

Also, as far as getting the code to work you can take a look at this jsFiddle which uses your code with a few modifications: http://jsfiddle.net/b9Fua/

Community
  • 1
  • 1
jeremysawesome
  • 7,033
  • 5
  • 33
  • 37
0

I find jQuery incredibly time-saving and intuitive. You just have to get used to throwaway functions. Check it out! http://jsfiddle.net/wgxZu/1/

<style type="text/css">

ul {
    list-style-type:none;
}

</style>

<body>

<div id="slideShow">

    <ul>
        <li>
            <img src="http://placekitten.com/100/100" alt="Steam Boat" id="boat" />
        </li>   
        <li>
            <img src="http://placekitten.com/200/100" alt="Steam Boat" id="boat" />
        </li>   
        <li>
            <img src="http://placekitten.com/300/60" alt="Steam Boat" id="boat" />
        </li>   
    </ul>

</div>

<script type="text/javascript" src="jquery.js"></script>
<script type="text/javascript">

    var image = document.getElementsByTagName('img');

    for (i = 0, ii = image.length; i < ii; i++)  {
        image[i].style.opacity = "0.5";
        $(image[i]).hover(function(){$(this).css({"opacity":"1.0"})},
                          function(){$(this).css({"opacity":"0.5"})});
    }
</script>

</body>
</html>
Tina CG Hoehr
  • 6,721
  • 6
  • 44
  • 57
0

You need to wait for the dom to finish loading before accessing any elements. Wrap your code in with the following:

window.addEventListener('load', function () {
    . . . // Your code
});

Additionally, your i variable is wrapped in the closure of both imageIn and imageOut. This means that whenever any image is receives a mouseover or mouseout event, the opacity will always change for the last image in your list of images.

To fix this, you can bind a scope to the functions:

image[i].addEventListener('mouseover', imageIn.bind(image[i]), false);

Then in your imageIn function you would do:

this.style.opacity = "1";

One last point: you are passing the string 'false' as the third argument to addEventListener. In JavaScript, any non-empty string will evaluate to true, so you should pass the boolean value false instead to prevent event bubbling.

pbeardshear
  • 910
  • 7
  • 8