daattali / shinyalert

🗯️ Easily create pretty popup messages (modals) in Shiny
https://daattali.com/shiny/shinyalert-demo/
Other
242 stars 26 forks source link

Issues with `timer` when there are multiple modals #31

Closed daattali closed 4 years ago

daattali commented 4 years ago

Bug 1:

When a modal has a timer and calls another modal in its callbackR, the modal from the callback doesn't show up

library(shiny)

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

server <- function(input, output, session) {
    observeEvent(input$test, {
        shinyalert::shinyalert("modal 1",
                               timer = 1000,
                               callbackR = function() {
                                   shinyalert::shinyalert("modal 2")
                               })

    })
}

shinyApp(ui, server)

Bug 2:

When a modal has a timer and there is another modal called in the same code chunk, the timer is ignored (this one might be ok because it's a bit ambiguous what should happen when two modals are called in the same code chunk - should the first one be closed before the second is shown? Or should the second be shown immediately?)

library(shiny)

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

server <- function(input, output, session) {
    observeEvent(input$test, {
        shinyalert::shinyalert("modal 1", timer = 1000)
        shinyalert::shinyalert("modal 2")

    })
}

shinyApp(ui, server)
dipterix commented 4 years ago

The Bug2 is because modals are queued internally in JavaScript library you use. The alerts are queued so that the second alert appears only after the first one is closed. In current implementation, you use shinyalert.num to mark the total number of alerts being fired. In this example, when the first alert is opened, its value is 1. However, when the second alert is created, this value changed to 2. Once the timer runs out, this if statement will be false

setTimeout(function(x) {
      if (x == shinyalert.num) {
        swalService.close();
      }
    }, params['timer'], shinyalert.num);

shinyalert.num=2 now but x=1.

In my PR, I fixed this bug. Instead of using num to keep record of number of alerts, I used shinyalert.indices to store the swal ID number created by swal_id = swalService.swal(params, callback). Now you can close specific alert via swalService.close( swal_id );.

If you want, you can even store a key-value pair in the js code where the key is shiny inputId created within shinyalert(), the value is swal_id so that if code writers want to close the first alert when the second is called, they can simply close the first one by closeAlert(first_inputId). (this feature is not implemented in my PR, but should be very close).

One use case is: let's say you have alerts A, B and, C, and C need to wait for B getting confirmed, but not A, code writers can show A, queue B, queue C and explicitly close A but leave B in the queue to display.

daattali commented 4 years ago

If your PR fixes this, could you please make a separate PR just for this (so that the PR won't have two features in it)? This would be great

dipterix commented 4 years ago

That's a by-product to fix bug2. As for bug1, you should use swalservice.closeAndFireCallback instead of swalservice.close. However, your swalservice.js file has a bug, closeAndFireCallback raises error when this.currentSwal is undefined. It also closes currentSwal prior to callback, which can cause wrong callback executed. I fixed that issue in my most recent PR.

daattali commented 4 years ago

Closing this issue in favour of two separate issues