First of, for someone learning jQuery and JavaScript, you've done a great job of avoiding the pitfalls of most new developers to JavaScript;
- You're not passing strings to
setTimeout
, which a lot of people do. - You've got a good grasp on closures (no matter how long they took you to learn :)).
- You're using strict equals (
===
) rather than equals.
... so, if you want me to be really, really picky;
I don't see much point in adding your code to the jQuery namespace. It would work just as well to be added to your own namespace. A lot of people fall into the habit of defining everything on
$
, and are scared of declaring your own namespace; don;t be:var ME = {}; ME.notify = function (title, msg, duration) { return new notification(title, msg, duration); };
It's a code convention to use a capital letter for constructors (e.g. functions you need to call
new
on); changefunction notification
tofunction Notification
.You could make use of prototypical inheritanceprototypical inheritance. As it stands, you're defining and adding a
hide
function on every instance of a notification you create. Obviously this has a negligible memory impact. Instead, use prototypical inheritance, and declare the function once;function Notification (blah, blah, blah) { // blah blah blah } Notification.prototype.hide = function () { var self = this; this.element.fadeOut(1000, function () { self.element.remove(); }); }
... you'd then have to change your double click handler to use the anonymous function approach I recommended in my answermy answer, as this
will no longer be the correct this
.
You've got a potential XSS exploit. For example, given the
message
:<script>alert(document.cookie);</script>
This will be injected, and evaluated in your page. You can fix this by setting the h3
and p
using text()
explicitly;
this.element = $('<div class="notification"><h3></h3><div class="notification_body"><span class="text"></span><br/><i>(double click to hide)</i></div></div>');
this.element.find('h3').text(title);
this.element.find('span.text').text(message);
... but again, I want to re-iterate that I'm being very, very picky. What you've got is well written, functioning JavaScript.