daattali / shinyalert

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

Add session as param for shinyalert function #52

Closed galachad closed 3 years ago

galachad commented 3 years ago

It's better to parametrize session in case where shiny::getDefaultReactiveDomain() is not working.

daattali commented 3 years ago

I'm not opposed to this, but have you run into this situation or can you think of cases where this is needed?

galachad commented 3 years ago

Yes, here three examples: 1. You can send alert to session that is not part of current ShinySession content. e.g. global observer or communication between session on the same process. 2. Some danger solution that's prototyping R6 ShinySession object. e.g.

shiny:::ShinySession$set(
  "public", "unhandledError", overwrite = TRUE, function(e) {
    # e.g. custom error handling to give more information to user
    # In this case you can not get session by shiny::getDefaultReactiveDomain() as it returns `NULL`
    # The session here is self
    # We can always send custom message e.g. by using self$sendCustomMessage('shinyjs-runjs', message = list(code = 'jscode'))
    # By I like simplicity here. 
    # shinyalert(..., session = self)
    # runjs(code = 'jscode', session = self)
})

3. Automated testing but this is very similar to first and second example as code is called outside or inside R6 of ShinySession domain.

It will be perfect to add session as argument to all your packages but some of them needs a little bit of refactoring as they are more complex.

By the way, thank you for all your contribution to R Shiny. 💯

daattali commented 3 years ago

This makes sense, I can see the value. Thanks!

Could you change the param documentation to "Shiny session object (only for advanced users)", and also run roxygen and add an entry to NEWS?

daattali commented 3 years ago

@galachad thanks for the idea, due to inactivity in this PR I ended up doing this commit myself. I also made a few more changes to it. I thanked you in the NEWS file for the idea.