Setting onclick function to <li> element

Question

I am trying to dynamically add onclick function to "li" tagged elements. But the event does not fires. Here is my code:

var arrSideNavButtons = [];
var sideNavLi = document.getElementsByClassName('side-nav')[0].getElementsByTagName('li'); 
var arrayOfSceneAudios = [scene1Audio, scene2Audio,...];

for (var i = 0; i < sideNavLi.length; i++) {
    sideNavLi[i].onclick = function() {
        arrayOfSceneAudios[i].play();
    }
    arrSideNavButtons.push(sideNavLi[i]);
}

Is it possible to code it this way? If yes, what is my mistake? Thanks a lot.

possible duplicate of Javascript closure inside loops - simple practical example — Felix Kling
– Felix Kling, Commented Nov 30, 2013 at 1:08
Good. If you already look at the console anyway, is there an error message by any chance? If not, odds are that sideNavLi is an empty list. If you are certain that it's not, there is not much else we can say. You should create a jsfiddle.net demo which reproduces the problem. — Felix Kling
– Felix Kling, Commented Nov 30, 2013 at 1:15

tymeJV · Accepted Answer · 2013-11-30 01:02:19Z

2

Wrap your onclick handler in a closure, else it only get assigned to the last elem in the loop:

for (var i = 0; i < sideNavLi.length; i++) {
    (function(i) {
        sideNavLi[i].onclick = function() {
            arrayOfSceneAudios[i].play();
        }
        arrSideNavButtons.push(sideNavLi[i]);
    })(i)
}

answered Nov 30, 2013 at 1:02

tymeJV

105k14 gold badges165 silver badges158 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

user2998173 Over a year ago

Don't work for me. Did you test it before? I mean, is it problem with me or still in the for loop?

Felix Kling Over a year ago

@user2998173: Learn how to debug JavaScript. Set breakpoints, inspect variables. We can only work with what you told us. It's on you to make sure that sideNavLi actually contains a set of elements. Similar for arrayOfSceneAudios.

user2998173 Over a year ago

Thanks for the link. I have console.logs every step, i just delete them here to make code shorter. The vars are ok.

user2998173 Over a year ago

You were right about setting onclick only for last element. Also the problem was in another function which overwritten this function. Thanks!

user2998173 Over a year ago

@Felix Kling can you help me in logical part? I have setted play() function for every button, but when i click on next one, they play one over another. How i can pause it before starting play a new one? I don't want to create a new post. Thanks!

|

Oriol · Accepted Answer · 2013-11-30 01:17:25Z

0

I think it's better to reuse one single function, instead of creating a new one at each iteration:

var arrSideNavButtons = [],
    sideNavLi = document.getElementsByClassName('side-nav')[0].getElementsByTagName('li'),
    arrayOfSceneAudios = [scene1Audio, scene2Audio,...],
    handler = function() {
        this.sceneAudio.play();
    };

for (var i = 0; i < sideNavLi.length; i++) {
    sideNavLi[i].sceneAudio = arrayOfSceneAudios[i];
    sideNavLi[i].onclick = handler;
    arrSideNavButtons.push(sideNavLi[i]);
}

answered Nov 30, 2013 at 1:17

Oriol

291k71 gold badges459 silver badges535 bronze badges

1 Comment

Oriol Over a year ago

@FelixKling Interesting link, but my answer doesn't extend the Element interfaces, it only adds properties to single elements. It still discomforts me a bit, and html5 data-* attributes containing the index i could be used instead, but I find this is clearer and shorter.

Collectives™ on Stack Overflow

Setting onclick function to <li> element

2 Answers 2

6 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related