Close the notification after destroy #10

Merged
merged 2 commits into from Mar 12, 2014

Projects

None yet

2 participants

@fcsonline
Contributor

It's interesting to close/hide a desktop notification after destroy it to not be overflowed by them if you create a set of them.

@fcsonline
Contributor

It could be interesting to add a new property called timeout to close it automatically after specified number of seconds. What do you think about?

@alexgibson
Owner

destroy() is only called when a notification is closed or there is an error, so I'm not sure if calling close again here really makes sense.

If you want to automatically close a notification after it is shown you can use something like:

notifyShow: function () {
    setTimeout(function() {
        notification.close();
    }, 3000);
}

You can also use the tag property to control single / multiple instances of a notification.

http://www.w3.org/TR/notifications/#close-steps

@alexgibson
Owner

Also, I would be open to adding a timeout property that called close for convenience here.

PR's welcome :)

@fcsonline
Contributor

Cool! But the method close is not in the notification prototype. Do you want to add it?

@alexgibson
Owner

Ah, correct - yes sure open to adding a close method (let's just leave destroy to clearing up after a notification).

@fcsonline
Contributor

Ok. I'm fixing it.

@fcsonline
Contributor

Check this out.

@alexgibson
Owner

Looks good to me 👍

I'll check it out locally soon. Would be good to add a JS test for this, but I can maybe do that later if you don't have time.

Thanks for the PR!

@fcsonline
Contributor

Thanks for adding tests! :)

@alexgibson alexgibson merged commit 59707ca into alexgibson:master Mar 12, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment