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

Dismiss modal after CallbackR finished #29

Closed
dipterix opened this issue Jul 4, 2020 · 8 comments
Closed

Dismiss modal after CallbackR finished #29

dipterix opened this issue Jul 4, 2020 · 8 comments

Comments

@dipterix
Copy link

dipterix commented Jul 4, 2020

Background

callbackR function is executed after modal being dismissed. One potential problem is if callbackR takes seconds to run, the Shiny apps will freeze up.

Simple example

library(shiny)

ui <- fluidPage(
  shinyalert::useShinyalert(),
  actionButton('a','Ok')
)

server <- function(input, output, session) {
  observeEvent(input$a, {
    shinyalert::shinyalert(
      'Warning',
      'Click OK to run code',
      callbackR = function(...){
        # procedure runs for 3~4 seconds
        Sys.sleep(3 + runif(1))
      }
    )
  })
}

shinyApp(ui, server)

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 function dismissalert to remove arbitrary number of modals.

The working example looks like this:

library(shiny)

ui <- fluidPage(
  shinyalert::useShinyalert(),
  actionButton('a','Ok')
)

server <- function(input, output, session) {
  observeEvent(input$a, {
    shinyalert::shinyalert(
      'Warning',
      'Click OK to run code',
      callbackR = function(...){
        # procedure 1 runs for 3~4 seconds
        Sys.sleep(3 + runif(1))
        # Now dismiss modal 
        shinyalert::dismissalert() # <------------- new function to remove alerts
      }
    )

    # A second modal right after the first one, once user 
    # press OK button, pop up message and show the code is running
    # Once finished, remove all alerts.
    shinyalert::shinyalert(
      'Scheduled!',
      'R is running, please wait for the result...', 
      showConfirmButton = FALSE
    )
  })
}

shinyApp(ui, server)
@daattali
Copy link
Owner

daattali commented Jul 7, 2020

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 #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?

@dipterix
Copy link
Author

dipterix commented Jul 7, 2020

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:
image

Once "OK" button is clicked the second alert pops up.

image

When dismissalert is called, the second alert gets automatically dismissed.

@daattali
Copy link
Owner

daattali commented Jul 7, 2020

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?

@dipterix
Copy link
Author

dipterix commented Jul 7, 2020

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.

@daattali
Copy link
Owner

daattali commented Jul 7, 2020

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 #31

@dipterix
Copy link
Author

dipterix commented Jul 8, 2020

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.

@daattali
Copy link
Owner

daattali commented Jul 8, 2020

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!

@daattali
Copy link
Owner

daattali commented Sep 9, 2020

Fixed with #37

@daattali daattali closed this as completed Sep 9, 2020
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

No branches or pull requests

2 participants