-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Remove toast explicitly even on focus #220
Comments
@jstawski Wanna open a PR for this? @TimFerrell do you agree that remove and clear should do this even if focus is on the toast? |
I can understand the use case, but I'm not entirely sure why that guard around the focus existed to begin with. I'd like to figure the history behind it before removing it outright. Additionally, I'm hesitant to make clearing a focused item if that's the functionality devs have come to rely on. In the event that we change the default functionality, should we introduce an option to restore the older functionality? Might be slipping into over-configuration, though. |
My thought was that if you are hovering over the toast, it should stick around. Think of how an outlook or growl notification works ... if you want it to stick around longer so you can read it, you hover, then it resets the timeout. So I want to keep that here, because without that delay the toast would disappear to soon. But I see Jonas' point. If he explicitly clears it, perhaps it should go away. I'm not sure of the exact use case for that tho. @jstawski can you clarify? |
@johnpapa @TimFerrell I think we should add a boolean paramater to the clear/remove functions that allow it to check for focus or not and default it so it is backward compatible. Another option would be to add more API level functions for explicit clearing and it would not be used by the timeouts. EDIT: Toastr works great with the use case mentioned above. I got confused it with another problem we were having. The real use case is when you want a toast to have a button that removes itself. Read the comments below for a jsfiddle example. |
I like the idea of overriding the clear function. |
@jstawski - care to create a PR with the modification? |
@TimFerrell will definitely create one, do you have any documentation I should follow for this project for PR? Unit tests, naming conventions, standards? |
@TimFerrell @johnpapa by the way here's a jsfiddle of the problem in action. A toast can't clear itself out with a button. |
Closing because PR was merged. |
Looking at the source code the toast doesn't get removed if there is focus on an element. I imagine that was done so the toast wouldn't get cleared automatically by the timer if you are working on it.
Shouldn't we allow the removal of the toast if we explicit call the remove? After all I am the developer and I know what I'm doing.
By the way the same thing happens with
clear
The text was updated successfully, but these errors were encountered: