daattali / shinyalert

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

Readme: change param name in callbacks #43

Closed drzamich closed 3 years ago

drzamich commented 3 years ago

Rename x to input for better example code readability.

drzamich commented 3 years ago

@daattali I was also thinking about simplifying if statements in lines 229 and 230.

-      callbackR = function(input) { if(input != FALSE) message("Hello ", input) },
+      callbackR = function(input) { if(input) message("Hello ", input) },
-      callbackJS = "function(input) { if (input !== false) { alert('Hello ' + input); } }"
+      callbackJS = "function(input) { if (input) { alert('Hello ' + input); } }"

What do you think about that?

daattali commented 3 years ago

I agree x is not great. But I would not use input, since that would override the input that the server function already has defined. Let's use something that isn't already defined.

Regarding the if statements: I prefer to be explicit

drzamich commented 3 years ago

I changed the param name from input to response. What do you think about that, @daattali?

daattali commented 3 years ago

Great choice. It looks like you missed a couple places where input is still used

drzamich commented 3 years ago

Right, I totally overlooked it! Should be fine now. I also changed value to response in yet another example to have consistency throughout the whole Readme.

daattali commented 3 years ago

Thanks, looks good. I forgot to mention before that the README is actually generated from the Rmd in the vignettes folder, so these changes need to be reverted and they need to be made in that vignette instead. Sorry for not bringing this up before!

daattali commented 3 years ago

@drzamich do you want to finish this PR?

drzamich commented 3 years ago

@daattali yes, sorry, it completely slipped my mind. I will finish it in the upcoming days

drzamich commented 3 years ago

Have to abandon this due to lack of time.