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

Adding a "refresh timer" to the scope #179

Closed
wants to merge 3 commits into from
Closed

Adding a "refresh timer" to the scope #179

wants to merge 3 commits into from

Conversation

jcford73
Copy link
Contributor

Adding a method to restart the timeout timer to keep a toast visible longer.

@Foxandxss
Copy link
Owner

A toast is an alert, to say something, for me, it doesn't make sense to restart the timer.

If an user needs to read it, they can hover it so it doesn't go away.

@jcford73
Copy link
Contributor Author

My use case is uploading multiple files. Instead of opening a screen full of toasts to announce that each uploaded successfully, I'm showing one toast and if it's still visible when the next file finishes, I'm updating the message and resetting the timer.

@Foxandxss
Copy link
Owner

I get you. I will think about that.

@jcford73
Copy link
Contributor Author

Do you know of any other way to accomplish what I'm trying? Currently, the toast continues to update until it times out and then another one immediately pops up with a new timer. It works okay, but the flicker when it has to pop a new toast is just a bit ugly.

@Foxandxss
Copy link
Owner

I would make a permanent toast for the time being and after all upload finishes, kill the toast.

@jcford73
Copy link
Contributor Author

Yes that would sort of work. Except then if there were only a few small files, the toast would disappear too quickly.

@Foxandxss
Copy link
Owner

I want this option, but having a method in the current scope is just one step, To access it you need to myToast.scope.refreshTimer() and I don't really like that API.

Also, I need tests.

@jcford73
Copy link
Contributor Author

I agree about having to go through the scope property to refresh the timer.

To be consistent with the rest of the API, it should probably go through the service like toastr.refreshTimer(toast) unless you want a shorter method name. Maybe toastr.extend(toast)? Or toastr.prolong(toast)?

@Foxandxss
Copy link
Owner

Perhaps just "restart"?

@jcford73
Copy link
Contributor Author

Could that give the impression it affects more than just the timer?

@Foxandxss
Copy link
Owner

You are right. Then refreshTimeout or simply refresh ?

@jcford73
Copy link
Contributor Author

I'll just keep refreshTimeout for maximum clarity.

Another topic: Tests are in toastr_spec.js but I haven't figured out how to run them locally.

@Foxandxss
Copy link
Owner

For the tests, you need to run testem. I need to swap that for karma.

So for today:

$ npm install -g testem
$ testem -f config/testem.json

The tests will watch any change that comes from $ gulp.

@Foxandxss
Copy link
Owner

Nice work on that, but we would need a few more tests cases where the toast is not there (long gone) and one where the toast was just gone one millisecond ago.

@jcford73
Copy link
Contributor Author

Will do.

@jcford73
Copy link
Contributor Author

jcford73 commented Aug 4, 2016

Are the tests sufficient? Are there any other test cases needed?

@Foxandxss
Copy link
Owner

Probably yes. Will double check tomorrow and release this weekend :)

@Foxandxss
Copy link
Owner

Thanks, I merged this. I will make a release.

@Foxandxss Foxandxss closed this Aug 20, 2016
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.

None yet

2 participants