Convert this "onclick" code in to a function

I have a JavaScript menu with around 50-60 menu items, the menu links work in conjunction with a SELECT form element, therefore I have assigned "onclick" actions on each of the links.

Here is my JavaScript function:

<script type="text/javascript">
function goto(url)
{
	var selection = document.getElementById('location');
	document.location = url + selection.options[selection.selectedIndex].value;
}
</script>

Code (markup):

And here is my <a href> code:

<div id="menu"><ul>
<li><a href="/directory/cd-players/" onclick="goto(this.href); return false">CD Players</a></li>
</ul></div>

Code (markup):

Now I know there is a way to simplify this so that the "onclick" action can be taken out of the a href and put in to the JavaScript function.

I'm not sure whether each link would need to have an ID attribute to achieve this? I can call each link in the CSS as .menu li a

 

<script type="text/javascript">

function getEvent (e) {
	var event = e || window.event;
	if( ! event.target ) {
		event.target = event.srcElement
		}
	return event;
}
function goto( e )
{
	var link = getEvent(e).target;
	var selection = document.getElementById('location');
	url = link.href;
	document.location = url + selection.options[selection.selectedIndex].value;
	return false;
}
function assignlinks()
{
	var menu = document.getElementById('menu');
	var links = menu.getElementsByTagName('a');
	
	for each (var link in links)
		link.onclick = goto;
	
}
window.onload = assignlinks;
</script>

HTML:

that should do it ..

it will add the even to all links inside the menu div

 

Cheers mate, I will try that when I get home!

Can you tell me what exactly the first function in your code above does?

 

most functions take by default a parameter which signifies the event that initiated them

Since we assign an action to the onclick event with this code

link.onclick = goto

Code (markup):

we need to be able to understand which element was clicked.
This function identifies the event (cross-browser) so we can extract from it the element that fired it up.

getEvent(e).target

Code (markup):

hth

 

mate, you shouldn't need this at all: in this context

function goto( e ) {
    console.log(this, this.getAttribute("href"), e);
... 

PHP:

this contains the clicked element, this.getAttribute("href") will give you the selection. e remains the event, should you wish to do anything to it

 

Very true ..

(have never played with events..)

So the revised code should be

<script type="text/javascript">
function goto( e )
{
    var selection = document.getElementById('location');
    var url = this.getAttribute('href');
    document.location = url + selection.options[selection.selectedIndex].value;
    return false;
}
function assignlinks()
{
    var menu = document.getElementById('menu');
    var links = menu.getElementsByTagName('a');
   
    for each (var link in links)
        link.onclick = goto;
   
}
window.onload = assignlinks;
</script>

HTML:

+rep on the way

 

I've wondered in the past if it was a bad habit to use "this" as an alternative to using the event object in the handler, since the context can be switched using call or apply and isn't really under control of the handling code. My concern is probably just theoretical and would probably never really cause a real problem. What do you think?

 

well, yeah - i personally feel that it is the lesser evil of the two. is the danger here is that the function won't work unless its raised by an element? then again, you control how it is being used whereas browsers can be twitchy about how they handle events... so all in all, if you can do w/o events, I feel it is the safer bet.

but to be honest, for the purposes of this script, i would have used an anonymous function anyway, something like:

// in vanilla js, avoiding function goto ...
for each (var link in links)
    link.onclick = function() {
        // do stuff with "this"....
    };
};

// or in mootools everything above can be done with...
$("menu").getElements("a").addEvent("click", function() {
    window.location.href = this.get("href") + $("location").get("value");
});

PHP:

another thing is, when you need to call on the function separately, you can bind "this".

(once again, examples in mootools):

...
console.log(this.get("href")); // outputs /directory/cd-players/

// setTimeout eqivalent:
(function() {
    console.log(this.get("href"));
}).delay(5000); // outputs null / undefined

(function() {
    console.log(this.get("href"));
}).delay(5000, this); // this is bound to context, outputs /directory/cd-players/

PHP:

another great thing mootools can do is bind functions to elements...

var myText = $("someid"), goRed = function() {
    this.setStyle("color", "red");
};


goRed.bind(myText); // bind the function to the element...

// now call it from any scope... won't matter
goRed(); // turns colour of $("someid") to red.

PHP:

not that i am saying mootools is great or anything

 

Hi again,

This works fine in Firefox but does not work in IE. I get the error message: Expected '('

This is on the line: link.onclick = goto;

Also the structure of the DIV menu is as follows:

<div id="menu">
<ul class="menulist" id="listMenuRoot">
	<li><a href="#">Weddings</a>
		<ul>
			<li><a href="#">Entertainment</a>
				<ul>
					<li><a href="/directory/djs/" onclick="goto(this.href); return false">DJs</a></li>
				</ul>
			</li>
		</ul>
	</li>
</ul>

Code (markup):

I only want the code to run on the two inner LI's ('Entertainment' and 'DJs'), and not the outer one (Weddings).

 

<script type="text/javascript">function goto( e ){ var selection = document.getElementById('location'); var url = this.getAttribute('href'); document.location = url + selection.options[selection.selectedIndex].value; return false;}function assignlinks(){ var menu = document.getElementById('menu'); var links = menu.getElementsByTagName('a'); for each (var link in links) link.onclick = goto; }window.onload = assignlinks;</script>

 

That's exactly the same code that gnp posted above. And as advised, it does not work for me in IE.

 

Anyone????

 

try this - if it fails, post a url with code and items in context (menu, links and location)

window.onload = function() {
    var menu = document.getElementById('menu'), selection = document.getElementById('location');
    for each (var link in menu.getElementsByTagName('a'))
        link.onclick = function(e) {
            window.location.href = this.getAttribute('href') + selection.options[selection.selectedIndex].value;
            return false;
        };
    };
}; // end onload - make sure you dont have any other onload / domready handlers or this will break. if so, move the code !

PHP:

also, you have <a href="/directory/djs/" onclick="goto(this.href); return false">DJs</a></li>

remove the inline onclick please, this duplicates the events set by onload...

 

Thanks for the post dimitar.

That again works in FF but not IE. I get the same error again: Expected '('

Just done some Googling and it seems that IE does not support for each loops in JS (this is where the error message points to)

Any chance you can rewrite the code using a different kind of loop (one that is compatible with IE)?

 

window.onload = function() {
    var menu = document.getElementById('menu'), selection = document.getElementById('location'), links = menu.getElementsByTagName('a');
    for (k in links)
        link[k].onclick = function(e) {
            window.location.href = this.getAttribute('href') + selection.options[selection.selectedIndex].value;
            return false;
        };
    };
}; 

PHP:

i think for ... in is standard.

sorry i forgot, so used to mootools' and its array.each(function(el, iterator) { etc...

 

Cheers dimitar, I managed to get that half-working on IE now. The problem is I can't have the code running on the outer menu, i.e:

<div id="menu">
<ul class="menulist" id="listMenuRoot">
	<li><a href="#">Weddings</a>
		<ul>
			<li><a href="#">Entertainment</a>
				<ul>
					<li><a href="/directory/djs/">DJs</a></li>
				</ul>
			</li>
		</ul>
	</li>
</ul>

Code (markup):

I only want the code to run on the two inner LI's ('Entertainment' and 'DJs'), and not the outer one (Weddings).

 

BUMP for dimitar

 

cheeky... what am I, a helpdesk?

I suggest you assign it a class like <a class="root" href="#">

then within the function go something like

        link[k].onclick = function(e) {
            if (this.style.className != "root") // do nothing if the link has class 'root'
                window.location.href = this.getAttribute('href') + selection.options[selection.selectedIndex].value;
            return false;
        };

PHP: