Hi Ben, I like your plug-in. It makes me giggle when I try to click the
buttons on your demo page... it reminds me of so many things in life. :)
Aside from that editorial comment I have some other feedback to use at your
discretion.
Personally, I am a big fan of spelling out the document.ready block. I am
often handing off my templates to other engineers who may not be
jQuery-savvy and using $(document).ready() is much more
understandable/intuitive than $( function(){} )... especially when you start
using $('string') in your $( function(){} ) ... that's where you can cause
confusion with the unenlightened.
$(document).ready(function(){
...
}); // close document.ready
Your inline comments are great! If it were my code I would also add a
comment block (JavaDoc style) above your plug-in that has
- what it does
- release/version number
- example usage
- what the license is (usually same as jquery, but good to be explicit)
- params in, returns out
- who authored it (including email)
- where someone can find out more info
Again, this is totally a personal choice. I also usually include a truncated
version of this in minified JS code (while other comments are completely
stripped)... just for the inevitable questions that come up when someone
looks at your code a year later.
It would be a good idea to make your plug-in $-friendly so if folks include
it on a site with Prototype (or other libraries) it won't go throwing
errors. Play nice with others!
(function($) {
$.fn.scared = function() {
...
}; // close scared
})(jQuery); // close $-safe plug-in
I think Jörn had a really good point with "this.OnMouseOver" ... I found it
confusing as well (at first I thought you were trying to bind the event up
there). My suggestion is functionally equivalent to Jörn's, just using a
slightly different syntax.
(function($) {
$.fn.scared = function() {
var handler = function(){...};
return this.mouseover( handler ).mousemove( handler );
}; // close scared
})(jQuery); // close $-safe plug-in
Since every jQuery method and plug-in should return the jquery object it
acted on, I prefer to return from the same line of code that I'm attaching
event bindings. It's possible and clean in something as simple as this
example. ymmv.
And I'm surprised no one else has mentioned this (they're being so nice)...
but no plug-in would be complete without configurable options! This is where
you get to have fun and figure out what folks might want to modify in your
script. For instance, you might want to have:
(function($) {
$.fn.scared = function( customOptions ) {
// default configuration options
var cfg = {
magnify: 1,
hitcount: false,
highscore: false
};
// override configuration options with user supplied object
cfg = $.extend(cfg, customOptions );
var handler = function(){...};
return this.mouseover( handler ).mousemove( handler );
}; // close scared
})(jQuery); // close $-safe plug-in
$('input.btn').scared({ hitcount:true })
Note: because you (now) have an object of default options already in the
plug-in they are not required. The $.extend method will just merge them
together with any customOptions overriding default options.
I'm looking forward to release 2 :)
Cheers,
Brian.
On 7/3/07, Jörn Zaefferer <[EMAIL PROTECTED]> wrote:
Hi Ben,
nice to see you on this list!
> Thanks for the feedback. I wasn't sure if the THIS scope was bad to
> use since it pointed to the jQuery object itself. But I guess, the
> whole point of the plugin is that you are extending the very nature of
> the jQuery object.
I haven't seen that approach before, so basically I'd say that approach
may yield some interesting results, or it doesn't bring any benefit in
contrast to just defining plugin-function local functions. In code,
something like this instead:
$.fn.plugin = function() {
function handler() {
...
}
$("...").bind("...", handler);
};
Binding the handler to the local scope increases its visibility a lot.
That could be handy at some point where you want to access it. But as
there is no need for it, you shouldn't do it. Adding to the local scope
could overwrite a prototype method with the event handler. Imagine a
handler-plugin:
$.fn.handler = function() { ... };
Now adding to this could actually have a bad effect:
$.fn.plugin = function() {
this.handler = function() {
...
}
$("...").bind("...", handler);
};
Because you are overwriting another method. $("...").plugin().handler()
wouldn't yield the expected result.
In addition, instead of defining an event handler and calling that
inside an anonymous function, you could just as well call the handler
directly. Inside all jQuery-event-handlers, "this" points to the
event-handling object, so inside your OnMouseOver you could replace
"jNode" with "this".
I'm a fan of writing simple code instead of numerous comments. Its a
win-win when you can replace a comment with a good method name that
explains the purpose and improves the code. The validator-object I use
within the validation plugin has over 30 methods, a lot of those only
containing one line. That helps a lot when extending the code, because
for almost everything there is a little method available that does just
what you need, no worries about copying code.
Another example of simplifying:
// Return the THIS pointer such that the jQuery chain
// is not broken.
return( this );
->
// don't break the chain
return this;
The switch could be simplified a lot by switching (sorry) to a map:
var directions = {
"0": { "top": intTop - intDistance }
...
}
jNode.css(directions[intDirection]);
You wouldn't even need a variable for that object (Untested, may be
screwed):
jNode.css({
"0": { "top": intTop - intDistance },
...
}[intDirection]);
Like written before, please add an alert to those buttons on click.
Makes that plugin a cool game. Add a hitcount and highscore and you're
done.
--
Jörn Zaefferer
http://bassistance.de
