1

I'm trying to write a script that can be run against any site, so i dont want to use any inline onchange approach. The script should fire any time there is a change to the inputCollection being created.

When I run the snippet below, I get an instant iteration and output of ALL the input fields and their value. But on "change" nothing happens.

So two issues: 1. I want it to only output the input that changed. 2. I want it to only run on the "change" event

Any insights in to what I'm doing wrong here? Here is a fiddle. http://jsfiddle.net/owq4n2eg/

 (function () {
 var inputCollection = document.getElementsByTagName("input");
 for(var i=0;i<inputCollection.length;i++)
 {
     inputCollection[i].addEventListener("change",console.log("name:" + inputCollection[i].name + ",value:"+inputCollection[i].value),false);

 };
 })();

Thanks!

Cyph
  • 623
  • 1
  • 7
  • 25

2 Answers2

4

Yea tymeJV's answer works but in my opinion it's not ideal.

(function(){
    var inputCollection = document.getElementsByTagName("input");
    for (var i = 0; i < inputCollection.length; i++) {
        inputCollection[i].addEventListener("change", handleFunction , false);
    }
})();

function handleFunction(event){
    console.log("name: " + event.srcElement.name + ', value: ' + event.srcElement.value);
}

jsfiddle

Getting the values off of the event.srcElement is much cleaner. Should be using the inputCollection only to assign the listener, not for output values.

Ryan
  • 5,644
  • 3
  • 38
  • 66
  • Unless we create a closure to preserve the value of `i`, `inputCollection[i].name` will always refer to the same element according to OP's code... [How does closures work](http://stackoverflow.com/questions/111102/how-do-javascript-closures-work) will be a good read... – T J Oct 10 '14 at 16:19
  • @TJ I know how closures work :P but that's why I said mine was cleaner. no need for closure at all cause your not using the value of `i` for output. – Ryan Oct 10 '14 at 16:22
  • If you know how closures work, Then why did you post: *"Also not sure what he's talking about with wrapping the loop in a closure"* ?? You could've said something like *"in my opinion, my solution will be better than using a closure*". instead. – T J Oct 10 '14 at 16:26
  • @TJ I re-read my answer and saw that it could be confusing so I removed that part. And that second part is basically what I'm saying below the code block. – Ryan Oct 10 '14 at 16:27
0

Well, you need to wrap your loop in a closure, else you'll always be using the last element. The listener also takes a function:

 (function () {
     var inputCollection = document.getElementsByTagName("input");
     for (var i = 0; i < inputCollection.length; i++) {
         (function(i) {
             inputCollection[i].addEventListener("change", function() {
                 console.log("name:" + inputCollection[i].name + ",value:" + inputCollection[i].value);
             }, true);
         })(i)
     }
})();

Demo: http://jsfiddle.net/owq4n2eg/3/

tymeJV
  • 103,943
  • 14
  • 161
  • 157
  • You don't need the IIFE in the for loop, your already in local namespace. On top of that your declaring a brand new function each time and crudding up the memory. – nepeo Oct 10 '14 at 16:20
  • @nepeo that is a closure for preserving the current value `i`. [How does closures work](http://stackoverflow.com/questions/111102/how-do-javascript-closures-work) will be a good read... – T J Oct 10 '14 at 16:22
  • a better solution would be to add an extra property to the element, and log the property using 'this' – nepeo Oct 10 '14 at 16:22
  • @nepeo Yes, that'll be a better option but this the *fix* for OP's code and it does work... so i see no reason for a downvote. – T J Oct 10 '14 at 16:24
  • That is very true, but @ryan answer helps the OP to understand context works better and encourages better practice. It doesn't just patch the issue. – nepeo Oct 10 '14 at 16:30