Any suggestions on my code :)

[Bob90]

Hello,

I want to know what you think of my code, including design, functionality and coding (If you can be bothered to view my external JS, I know I'm not making this easy)

Last time I asked it didn't work in Konqueror. I have only tested it in IE and FF.

http://www.robertgarford.com/code/ra...1_2_strict.htm


Many thanks to anyone who helps. Even if its not constructive!

[Twey]

It works in Konqueror now. The design's nice, the functionality's good, but the code needs work.

//Browser detection
var ie5=document.all&&document.getElementById;
var ns6=document.getElementById&&!document.all;

Argh! Don't use browser detection!

		document.all["radialMenu"].innerHTML = menuContent;

innerHTML is non-standard and often produces unexpected results. Use DOM functions instead. The same applies to createContextualFragment().

			menuContent+="<a href=\""+tempAr[0]+"\" target=\""+tempAr[1]+"\" onmouseover=\""+rolloverTxtOn+scaleTxtOn+",goTo='"+tempAr[0]+"'\"

You mean ; not , here.

<img id=\"id0\" width=\""+circle_menu_item_dimension+"px\" height=\""+circle_menu_item_dimension+"px\" src=\""+picDir+"blankbg.png\" style=\"border:0px; position:absolute; left:"+(parseInt((circle_menu_dimension/2)+(circle_menu_radius*Math.sin(y*min_angle_between_items)))-parseInt(circle_menu_item_dimension/2))+"px; top:"+(parseInt((circle_menu_dimension/2)-(circle_menu_radius*Math.cos(y*min_angle_between_items)))-parseInt(circle_menu_item_dimension/2))+"px;\" />

You use the deprecated width and height properties (which don't take units except %, by the way), even though you're using CSS anyway. You also use an XHTML-style self-closing tag, even though the script won't work on XHTML pages due to your use of innerHTML.

eventObj = e? e:event;

Unnecessarily (and pointlessly, since the event will expire) global object. Could cause all sorts of problems. There are a lot of global functions in your code. You might want to consider putting the whole thing inside a namespacing object of some kind.

if(window.addEventListener)
	{ // Mozilla, Netscape, Firefox
	window.addEventListener('load', startup, false);
	} 
else 
	{ // IE
	window.attachEvent("onload", startup);
	}

What if neither exist?

//Event handlers
function setNavType()
	{
	if (ie5||ns6)
		{
		menuobj=document.getElementById("radialMenu");	//Get menu div
		menuobj.style.display='';							//Hide menu
		document.oncontextmenu=function(){return false}; 	//Disable Browser Context Menus
		
		if(proNav)
			{
			document.onmousedown=showMenu;
			document.onmouseup=function(){followLink(),hideMenu()};
			}
		else
			{
			document.onmousedown=showMenu;
			document.onmouseup=function(){followLink()};
			document.onclick=hideMenu;
			}
		}
	}


function startup()
	{
	setNavType();
	displayEvenly();
	hideMenu();
	}

So you rely on DOM events to attach to load, but overwrite any mouse events on the document anyway?

[Bob90]

Firstly,
Thankyou so much. Having someone like you look over my code is amazing.
The next thing I was going to change was the event handling, but you are fantastic. I really didn't expect anyone to give that much constructive critism.

Anyway enough praise. I will look at all your suggestions and make all the appropriate changes.

I like being a regular here. If someone asked me to look at their code in a separate .js file I would just ask them to post the JS. Ah well, maybe I need to change my manner.

Thanks again.

Rob

[Twey]

If someone asked me to look at their code in a separate .js file I would just ask them to post the JS.

For me, at least, it's easier to debug running code. If someone pastes a JS file, I have to construct a working demo page before I can debug it thoroughly, so I prefer live demos.

[mburt]

I also try to use what the original script looked like, and to work around that. This will probably help the original poster see how their code was changed.