Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification queue #11

Merged
merged 4 commits into from
Sep 20, 2011
Merged

Notification queue #11

merged 4 commits into from
Sep 20, 2011

Conversation

sgreenfield
Copy link
Contributor

Your notification plugin is the best I've been able to find so far, but it was missing one key feature that we required in order to use it. I have resolved this issue in my fork.

This fork allows the plugin user to limit the amount of notifications visible at one time. The notifications that have been created after this limit has been reached are displayed in order (first in first out) when the previous notification(s) are closed.

Hope you like it :)

-Scott

@ehynds
Copy link
Owner

ehynds commented Sep 19, 2011

Hi, I dig the feature. One question I saw after a quick first glance. Once the queue is dequeued, won't it then display ALL notifications that have been queued, not just the next 6 or however many have been configured?

@sgreenfield
Copy link
Contributor Author

Nope, it still stays within whatever limit was set. It simply uses the next item in queue as each item closes. Try opening limit-example.htm and clicking the input button like 100 or so times :) You'll see that it only fills up the limit.

Hope this helps,

Thanks,

-Scott

@@ -48,8 +49,8 @@ $.widget("ech.notify", {
tpl = $(tpl).removeClass("ui-notify-message-style").wrap("<div></div>").parent().html();
}

// return a new notification instance
return new $.ech.notify.instance(this)._create(msg, $.extend({}, this.options, opts), tpl);
$.ech.notify.openNotifications = $.ech.notify.openNotifications || 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to this.openNotifications = this.openNotifications || 0 ?

@ehynds
Copy link
Owner

ehynds commented Sep 20, 2011

Good deal. I made some line comments just for consistency. If you can update those & make sure it still functions properly (i.e., my comments didn't break anything) I'll merge it in. thx!

…xample to index.htm to show that it works with the other examples. Updated minified JavaScript to reflect the changes.
@sgreenfield
Copy link
Contributor Author

Hopefully this is what you're after. :)

@ehynds
Copy link
Owner

ehynds commented Sep 20, 2011

Yup that was a typo on my part.

ehynds pushed a commit that referenced this pull request Sep 20, 2011
@ehynds ehynds merged commit 9057b6f into ehynds:master Sep 20, 2011
this.element.fadeTo(speed, 0).slideUp(speed, function(){
self._trigger("close");
self.isOpen = false;
self.element.remove();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Not sure if you noticed this little change I sort of snuck in, but I think this is pretty important because the user could theoretically end up with thousands of unused DOM elements, each with an instance of notify() stored in memory. Could possibly cause memory issues if a user keeps the window open for days and never refreshes the page. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it makes a lot of sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants