PDA

View Full Version : :) What do you think of the code now?



Bob90
05-22-2007, 12:00 PM
http://www.robertgarford.com/code/radial-menu/radial%20menu%201_5_strict.htm

I've been through the code and OO some of it. Will finish the rest after exams.
Rewritten to remove browser detection.
Added fall backs in certain situations, like eventhandlers.
Used mostly DOM2(?) methods.

Is there anything obvious/stupid that I have/haven't done.

Thanks in advance.

:)

Trinithis
05-22-2007, 04:09 PM
I think its pretty cool. Perhaps I missed something, but I'm assuming it's just the radial menu you coded. Though, I found the text on the page background hard to read and had to hit ctr+a. I do not fully understand the red "Here" box floating in the middle of the page in one part. I noticed that when you tell the radial menu to kill itself, it docks there, but I haven't seen it do that elsewhere. If that is intentional, I would suggest that you would be able to move the red docking area with your mouse or something so it does not block the page.

Right now I think I found a bug (BTW im using FF 1.0.7 on Windows). When you tell the menu to kill itself and the code sends it to the red box, you can see a miniturized menu there and you can click on it to bring back the menu. If you select kill menu again, it puts in the mini-menu into the box along with an extra mini-menu picture. Doing this many times yeilds more and more.
(My mouse is hovering over one, but the Print Screen did not capture it.)
http://img.photobucket.com/albums/v251/Trinithis/Radial.jpg

One suggestion is to add like a new window or a new tab option for the links in the menu.

Bob90
05-22-2007, 05:21 PM
Hey, thanks for your input. :D
I had already found that 'bug'. You can see my comment on it in the code. What really interesting is that the bug only affects FF, not IE and if you undock then click somewhere else, then re-dock the menu it doesn't happen! Weird. I will look at my code more thoroughly after my exams.
EDIT: In fact, I may post a new topic on why FF does this... may be a bug!

my code comment

//Dock to div only show one pic
slightly cryptic

As for opening in a new window. It already supports that :). Well it supports anchor target, so it can target certain frames, new windows or itself.

The reason the red div is floating is just to show that the menu can be docked to any div, anywhere! The user can place the div anywhere on their page.

Trinithis
05-22-2007, 05:32 PM
Well, I have a suggestion, but it might be too much work, but it could be fun. I read that you could insert any number of images into the menu, and if too many are inserted then it would have a "more" button. Well, my idea is that instead, the menu just tacks on more rings and evenly distrubutes options within each ring. If one or two options are in an outer ring, then the outer ring would commandeer at least X options from the previous ring to make the outer have at least three in its ring, triangulating them, etc.

Bob90
05-22-2007, 05:51 PM
Firstly, you've caught me out. That was left on from version 1.0, which I consider obselete. V1.0 has that option, but I have considered different ways to fit more items on the menu since then (it has to do with the way the user navigates).

Your idea is great and I still think it a possibilty.

The more I read, the more I think that, in fact, having submenus pop-up over the items would be more advantageous.

I will try and code your idea into version 2, which will be complete by August (Planned)

:D

blm126
05-23-2007, 03:09 AM
First off let me say this, I find the concept interesting, and it seems to work well. However, there are a lot of problems with the code that I see. So in no particular order...

-The menu syntax is weird. What if you want to use the | in the test or image. Sub-arrays would be smarter.

-Misuse of parseInt


this.index = parseInt(this.parent.items.length,10);

The above code doesn't need to be converted to a number, it should already be one, and even if it does need converted just use the Number constructor.

-IE only code?


if(document.attachEvent)
{
temp_obj = document.getElementById("id_a"+x);
//temp_obj.attachEvent("onmouseover",function(){rollover('id'+randId,picDir+tempAr[3]);highlight('id'+randId+'');goTo=tempAr[0];} );
//temp_obj.attachEvent("onmouseout",function(){rollover('id'+randId,picDir+tempAr[2]);lowlight('id'+randId+'');goTo='';} );
temp_obj.onmouseover = function(){rollOver(this);goTo=tempAr[0];};
temp_obj.onmouseout = function(){rollOver(this);goTo='';};
}

In the above code you seem to test for IE's attachEvent(and maybe Opera,can't remember), but then you use the old style of events, which is fine. But this either outdated code, or browser sniffing. Both are bad.

-Browser Sniffing


var ie5 = document.all;

Ok, now that is browser sniffing for sure. Use Conditional Compilation, it's safer.Opera, Firefox, and all versions of IE6-7 actually support document.all. This is only working because Firefox actively lies to you.

-Browser Sniffing(again)


if(navigator.appName == "Microsoft Internet Explorer" && parseFloat(navigator.appVersion.split("MSIE")[1]) > 5.5 && el.src.toUpperCase().substring(el.src.length-3, el.src.length) === "PNG") //if ie and png
{
el.style.filter = "progid:DXImageTransform.Microsoft.AlphaImageLoader(src='"+el.src+"',sizingMethod='scale')";
}
}

Use Conditional Compilation, trust me.
-Nested if-else-if-else staments


else
{
if(document.attachEvent)
{

Use else if, it's more readable.
-Why?


function returnFalse()
{
return false;
}



As you can see, my biggest complaint is that this isn't really cross-browser. IE support is held together by metaphorical duck tape(document.all). It's liable to break at any time. If you spend the time to make this truly cross-browser, it would be much better. Also, I found the formatting of all the code odd. Consistent spacing makes code more readable, but that is your choice.

StarrRider
05-23-2007, 03:52 AM
Personally - I like it!
Good Job
Lee

Bob90
05-23-2007, 07:59 AM
Thanks for the advice. :D

BLM126,

Nice catches. TY.

Some Questions/Points

1. Browser sniffing for PNG correction is a must. One of the above is for that reason.

2. why return false - Good Point! I can't remember why.

3. Why doc.attachEvent only to use obj.onmouseover = .... - I had problems dynamically adding eventhandlers to dynamically created elements in IE.
Do you know of the correct way?

4. Sub arrays - yeah I suppose so

jscheuer1
05-23-2007, 08:31 AM
Doesn't do a damn thing in Opera.

mwinter
05-23-2007, 04:00 PM
Doesn't do a damn thing in Opera.

Probably because the option "Allow script to receive right clicks" is disabled by default. :)

I haven't looked at the code, but I would condemn the script on principle alone because it interferes with the context menu.

Bob90
05-23-2007, 04:54 PM
Well thats useful! If you haven't looked I don't care for your opinion.

John, the code allows the use of any mouse button used in combination with any keydown so could can work with Opera.

mwinter
05-23-2007, 05:56 PM
Well thats useful!

I know.



If you haven't looked

Others had already highlighted issues that I would consider pretty damning. If you really want me to provide a critique, so be it.


Variables are used without prior declaration.
[style] Seems strange to use an array literal without actually taking advantage of the notation.
[style] Uses string comparison to implement enumeration type.
Little use of feature detection.
Little defensive programming.
Object constructor function returns this object: redundant.
Uses for..in statement to iterate array.
Pollutes global namespace.
Uses document.location, rather than global location object: possible compatibility issues.
[style] Uses childNodes[0] rather than firstChild property.
Uses setAttribute method, rather than property shortcuts.
Uses setAttribute method to attach event listeners.
[style] Uses typeof operator in combination with parentheses, but no expression is involved.
Uses parseInt function without a radix specifier.
Listener addition and removal is repetitive. Use functions to implement alternative code paths.
Assigns empty strings to event listener properties.
Uses setTime method to increment Date object by day intervals.

That's not necessarily an exhaustive list.



... I don't care for your opinion.

It wasn't an opinion.



John, the code allows the use of any mouse button used in combination with any keydown so could can work with Opera.

Which would surely interfere with other built-in interface features.

jscheuer1
05-23-2007, 07:20 PM
Doesn't matter what keys I press, still does nothing. I think I mentioned before in some connection to this project, as has mwinter, that trying to co-opt the context menu is a losing proposition.

mwinter
05-23-2007, 09:04 PM
Something I thought of not long after sending my previous reply was: why bother writing RadialMenu using a constructor function? There can be only one instance.

blm126
05-23-2007, 09:14 PM
1. Browser sniffing for PNG correction is a must. One of the above is for that reason.

I beg to differ. http://www.javascriptkit.com/javatutors/conditionalcompile.shtml

Also, there are many instances of Browser Sniffing, not just the PNG one.

Bob90
05-23-2007, 09:26 PM
Well, I'm back.

Bob90
05-23-2007, 09:36 PM
I know.
I hope you're being sarcastic too.

If you honestly think that a statement like this is constructive then you need help.


I haven't looked at the code, but I would condemn the script on principle alone because it interferes with the context menu.


So now because I tell you I don't want your advice you feel obliged to give it to me. LOL, I'll just tell you I'm ignoring you in the future too.

An honest thankyou to the list of problems. That is what I asked for in the first place. :)



That's not necessarily an exhaustive list.

Please don't hold back.



It wasn't an opinion.

Yet

"I would condemn the script on principle" - previous comment

So yes it is an opinion.

There will be the option of more than one menu in the future so the constructor will be neccessary.

As for the context menu, thanks for your advice, but I am still going ahead with the project. When finished it will not rely solely on the context menu being disbaled.

I fail to see how browser sniffing for a browser defect is wrong. Conditional comments don't even seem to have specific browser versions, just Jscript versions. I only want to alter PNGs on IE 5 and 6 and therefore I look at the navigator string. (Yes I know the code says IE>V5, that will get altered). It is a lot more to the point and who does it harm? Its used in conjuction with a search for image filters so anyone that spoofs the navigator will not call it.

mwinter
05-23-2007, 09:49 PM
I hope you're being sarcastic too.

No, I'm not. Just because you don't appreciate that co-opting the context menu is a bad idea (fact, not opinion) doesn't mean that the comment is any less justified.



So now because I tell you I don't want your advice you feel obliged to give it to me.

Did you consider that when I wrote my first comment,


I was primarily commenting on Opera's default behaviour in response to John's post, and
I didn't have the time to review the code in full.

jscheuer1
05-23-2007, 10:03 PM
Take it easy on Mike (mwinter). He knows more about coding than the rest of us in this forum combined. If he says taking over the context menu is bad just on principle and says that it is a fact, not an opinion, he is probably right.

Think about it for a few moments. It won't even work in any browser that disallows taking over the context menu. That is (if you include those where it is an option) up to about half of all installed browsers. Even on those browsers where the user has no choice, it can be quite annoying to have your interface 'stolen' by the designer. Almost all javascript does this in some minor way (rollovers, slide shows, etc.), but taking away an entire menu is going a bit far.

That's why it is a fact that it is a bad idea just upon principal. There may be additional reasons. And, surprisingly enough, no reading of the code is required to determine this.

Bob90
05-23-2007, 10:06 PM
If you could get someone with a degree or higher education in English to prove that your (In my sense) opinion is not an opinion then I will believe it is a statment of pure fact.

I believe it is an opinion relating to user interfaces, not a fact.
I also believe that if the context menu was so important that the majority of browsers would not let you disable it; especially with Javascript.
Maybe they believe there is some benefit that may come of disabling it?

EDIT: Surely reading the code or trying the menu is the point. If you had tried the menu you would see that the context menu is very easy to get back. Just navigate to the dock button. If I thought the average user of my site to be a technophobe, I could start the menu in docked state (Context menu available).

EDIT: Good with code doesn't neccessarily mean good with English.

Bob90
05-23-2007, 10:14 PM
Take it easy on Mike (mwinter). That is (if you include those where it is an option) up to about half of all installed browsers.

Don't distort the facts (;) ).

How about if we include if it is an default option. Which to my estimate makes the figure a lot closer to 100%

Bob90
05-23-2007, 10:20 PM
Did you consider that when I wrote my first comment,


I was primarily commenting on Opera's default behaviour in response to John's post, and
I didn't have the time to review the code in full.


1. Well, its hard to argue that the second line, which is the one in question, is primarily to do with Opera.

2. No, sorry, i guessed you just wanted to have a quick gripe at the person coding something that interferes with the context menu. I shouldn't have judged you like that, but I get it all the time. Especially from Mods and especially in short responses. I'm sure you can see why your comment was treated the way it was.

What!!!
http://www.dynamicdrive.com/dynamicindex1/contextmenu.htm
This just feels like I'm between a rock and.... well... context menu heaven. ;)

mwinter
05-23-2007, 10:59 PM
I believe it is an opinion relating to user interfaces, not a fact.

There are features presented in the context menu that are simply impossible to replicate. Some of these features are not available elsewhere. By disabling the context menu, you deprive users of that functionality. Even withstanding that, it is not appropriate to inconvenience a user, forcing them to take alternative measures to achieve what would otherwise be a quick and simple operation.



I also believe that if the context menu was so important that the majority of browsers would not let you disable it; especially with Javascript.

I know for certain that both Firefox and Opera do provide such an option, though evidentially the implementation in Fx is poor. I don't particularly feel like installing Linux to check *nix-specific browsers, and I don't have access to a Mac.



If you had tried the menu you would see that the context menu is very easy to get back.

I know, but that's not quite the point: it's still an impediment. Moreover, computer illiterate users (which are abound on the Web) aren't going to just be slowed down.



If I thought the average user of my site to be a technophobe, I could start the menu in docked state (Context menu available).

Yes, of course.



Well, its hard to argue that the second line, which is the one in question, is primarily to do with Opera.

I wasn't trying to: the expression of disapproval was a passing remark, stated only because no-one else had up to that point.



2. No, sorry, ...

:)



What!!!
http://www.dynamicdrive.com/dynamicindex1/contextmenu.htm
This just feels like I'm between a rock and.... well... context menu heaven.

Just because I post here doesn't mean that I approve of all of the available scripts.

blm126
05-24-2007, 02:18 AM
Browser Sniffing is bad because it doesn't work. Sure, it'll work in the browsers you test, but many browsers include parts of the IE DOM or forge their UA string to make them appear to be something they aren't.


var ie6orless = /*@cc_on @if(@_jscript_version < 5.7)true; @else false; @end*/false;

There you go, that should detect IE6 or less perfectly.

Bob90
05-24-2007, 08:03 AM
blm126, if you could tell me how to get browsers between 5.5 and 6.x then I will say its ok.

Like I said. I still check for the filters, so what is the real problem there?

jscheuer1
05-24-2007, 08:23 AM
Technically speaking, one should only check for filters to determine whether or not to use filters. The same is true of any other feature/method. However, IE - starting at version 5 makes it awfully tempting to do browser detection. With conditional comments you can create browser specific objects (or elements or whatever). To get just 5.5 through 6.x:


<!--[if gte IE 5.5]>

<![if lt IE 7]>

<script type="text/javascript">
var IE55to6x=new Object();
</script>

<![endif]>

<![endif]-->

<script type="text/javascript">
alert(typeof IE55to6x)
</script>

StarrRider
05-24-2007, 09:52 AM
Bob90
As I said earlier - I do like your script. The menu is ascetically pleasing and it is obvious that you've spent a lot of time on it.

With all of that said - please consider. I went to your site knowing that there was something to look at - found it and admired it. Aside from the fact that it steals the right menu (temporarily) / the main problem with it is the user. It's a hidden / unconventional menu that is not a very intuitive to the average user.

By that I means - something is always going to be needed to let the user know that it is there. You solved it by including the blurb:
Quick Start / Hold down the right mouse-button. / Navigate to an icon. / Release the mouse button."

Actually - you spent the entire page explaining it - and even then I had to read the comments on this page to realize what the small donut in the red box was. With all that text necessary to make a user aware of it and how it works - what does it gain you? Most of the menus on DD would do the same thing in a lot less space and not need to be explained.

In short - even though I do like your menu - I wouldn't use it. I couldn't recommend it - unless a client specifically asked for a look that was unusual - and I've seen quite a few that fit that description. It has been quite a while but there was another circular menu that used a onclick in the body of the page to make it appear (and it stayed there until another onclick happened) and the icons slowly rotated around it.

I do not mean that you should give up on it - at least not yet. Because it does look very good - it will open a few doors for you in the job market when you need them opened. Most of the people who see it won't know enough about the things mentioned here (I have a very low opinion about managerial types) - but a few will. Use the evaluations you've gotten and fix it. Then save it for that day - and the days after that.

mwinter :D
HeHeHe - I gotta tell ya - you really did yourself in this time.
Everybody who reads this thread is going to know how to get a really good flipping evaluation out of you! Can't you hear all those really large sharpies writing "Insult mwinter when finished"? I hope you don't have high blood pressure.

Bob90
05-25-2007, 08:22 AM
Hey,
Thankyou to all who gave constructive criticism. :D It is truly appreciated.

StarrRider, thanks for the heads up, I haven't considered how normal people would interact with the menu if they have never seen one before. I guess that's why nobody has done it before.

Ah well, bring on V2.0 then I'll let it die.

Thanks again,

Rob
:)

Twey
05-25-2007, 09:54 AM
The menu is ascetically pleasingI doubt it...
Ascetic \As*cet"ic\a.

Extremely rigid in self-denial and devotions; austere;
severe.
[1913 Webster]You probably meant "aesthetic" :) Apologies for nitpicking, it jumped out at me.

StarrRider
05-25-2007, 11:18 AM
Correct you are - and no apology needed. :)

I doubt it...You probably meant "aesthetic" :) Apologies for nitpicking, it jumped out at me.
Give me a minute to beat my spell checker again - it seems to be designed to produce this kind of error. Of course - it couldn't happened if I had learned to spell in the first place.
Now if you want a laugh - in this post it underlines 2 spelling errors. The first is your spelling of "aesthetic" and wants me to change to "anesthetic" and the second is "nitpicking" which it thinks needs to be hyphenated. Ahhhh - the conveniences of our computer driven age!

Twey
05-25-2007, 12:00 PM
Now if you want a laugh - in this post it underlines 2 spelling errors. The first is your spelling of "aesthetic" and wants me to change to "anesthetic" and the second is "nitpicking" which it thinks needs to be hyphenated. Ahhhh - the conveniences of our computer driven age!Oh dear! I can understand "nit-picking," since it would usually be hyphenated ("to nitpick" is a special case that's come to be recognised as a verb in its own right, although whether this would stand up to examination by major dictionaries I'm not sure), but there's definitely nothing wrong with "aesthetic," unless it still expects me to spell it "&#230;sthetic" :p