Closed dipterix closed 4 years ago
Thanks for the inititative to create the PR and the great example.
I did previously have a closeAlert()
function but I ended up removing it after a while because it didn't work in all cases (I don't remember the specifics though... I really should have documented why I removed it).
With your proposed solution, are you now required to call dismissalert()
in the callback function? I can definitely see the motivation for your case, but frankly I'm not sure I agree it's the correct thing to do. I think of callback as a function that's called after an event ends , not just before it ends. In my view, the modal should get dismissed as soon as your click on the close button, and if there's any work that needs to be done after, then it's up to the user to deal with the consequences of that job taking a long time to run.
This isn't to say that having a dismiss function isn't useful - if it works correctly then I would be happy to have that function. And with a dismiss function along with https://github.com/daattali/shinyalert/issues/26 , a user would be able to do what you want to do easily by adding an action button and running the callback+dismiss on that button instead of using the callbackR
parameter.
What do you think?
Thanks for the reply, please let me explain :)
With your proposed solution, are you now required to call dismissalert() in the callback function?
I didn't want dismissalert
to be mandatory, everything works exactly the same as before. If users have already written code based on shinyalert
, they don't have to change anything.
I think of callback as a function that's called after an event ends , not just before it ends. In my view, the modal should get dismissed as soon as your click on the close button, and if there's any work that needs to be done after, then it's up to the user to deal with the consequences of that job taking a long time to run.
My initial issue description might be confusing, I don't want to change anything that already exists, and I agree with you on callback should be called once button is clicked. However, if you look closely to the code, you'll find I have two alerts in a row. The first one shows the normal alert, once users click on the OK button, the first one is dismissed, callback gets called, in the meanwhile, the second alert pops up.
The second alert is what I want. It's a message alert that cannot be closed by hitting outside of the message box nor Esc
key. There is no button to close it. It's a pure message box that prevent users from doing anything while shiny is running stuff blocking the main process.
Here's the first alert:
Once "OK" button is clicked the second alert pops up.
When dismissalert
is called, the second alert gets automatically dismissed.
You're right that I didn't try your code - I tried running your first sample code, but the second one I thought wouldn't work because it uses the new function you added. Anyway, I would approach this a little differently: you want the second modal to be called after the first one as a callback. So I would do it as:
callbackR = function(...) {
shinyalert::shinyalert(
'Scheduled!',
'R is running, please wait for the result...',
showConfirmButton = FALSE
)
Sys.sleep(3 + runif(1))
shinyalert::shinyalert("Done!")
}
Or instead of the last shinyalert, a dismiss/close function would be appropriate there. So your PR for a dismiss can still be useful. But do you agree that this approach would be more fitting for your situation?
Oh, the first sample code doesn't have 2 alerts. I used it to purely display the issue/scenario: if you launch modal, dismiss the modal and press "Ok" button at once to launch alert again, because Sys.sleep(3)
is called, the modal won't show up. We know shiny
is busy and not responding, but users might be confused. In this case, it's be better to show an alert like "shiny is busy/work scheduled" so that users know something is happening.
Yes, your example fit in my case. The third alert shinyalert::shinyalert("Done!")
will not shown if users don't close second alert ('Scheduled!'
). Also if users close the second alert too soon, shiny is still busy and the third alert will be very confusing, hence it's useful to have dismissalert
for code writers to decide when the alert should expire.
I fully support having a dismiss function (I would prefer to call it closeAlert
) - it just needs to be tested thoroughly. I think that would be enough to support what you want to do, do you agree?
shinyalert shouldn't try to solve the generic problem of letting the user know when shiny is busy - that's a shiny issue that's out of scope for shinyalert.
This also made me think of another useful feature - adding a force
or immediate
paramter to shinyalert()
that would show the message right away and close any existing modals. What do you think?
I also discovered a couple bugs with timer
while doing all this https://github.com/daattali/shinyalert/issues/31
Cool! I like the name closeAlert
. I agree shinyalert
shouldn't try to resolve generic problem of letting the user know when shiny is busy, but with closeAlert
, the code designers can have more control over the workflows and people will be more willing to use your package because of that.
Adding option to set immediate=TRUE
is a good idea. I also commented #31 on Bug2. I realized it when reading your js code, so I fixed it in my PR. Try it out, and timer
should work now.
Please submit one PR for fixing the javascript, and one PR for adding the closeAlert
function. After those are implemented, immediate
can be added as well. Thank you for all your help!
Fixed with #37
Background
callbackR
function is executed after modal being dismissed. One potential problem is ifcallbackR
takes seconds to run, the Shiny apps will freeze up.Simple example
If you dismiss the modal and immediately press the "Ok" button, the app freezes around 3 seconds before opening the modal again. This greatly affects user's experience and making
callbackR
less attractive.My Solution
I modified your code a little bit, added a function called
dismissalert
to manually remove messages. I'll submit a PR soon, here is my forked repo. Basically I modified your JS code, and added R functiondismissalert
to remove arbitrary number of modals.The working example looks like this: