-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Improve documentation for submitButton and change 07_widgets example to use an action button #1475
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
Conversation
…and an full-app example)
shiny-examples PR: rstudio/shiny-examples#54 |
shiny-dev-center PR: rstudio/shiny-dev-center#32 |
@@ -4,6 +4,54 @@ | |||
#' button do not automatically update their outputs when inputs change, | |||
#' rather they wait until the user explicitly clicks the submit button. | |||
#' | |||
#' @section Warning: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be a warning -- just having the text in the default section (details) should be fine.
#' described in the link above): | ||
#' | ||
#' \preformatted{ | ||
#' shinyApp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to leave out this example, because I don't think people will understand how it's a translation of the original submitButton
version -- it's notably more complex, and uses a function (eventReactive
) they're likely not familiar with. I think the people will be better served by reading the article. For the same reason, I also changed the article link (above) to point to the top level of the actionButton article instead of jumping to the eventReactive
example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was also not feeling great about doing the "translation" right in the documentation. But since we're also moving the aside about submit buttons to the end of the Action Button article, do you think we could include both the example and the translation there (i.e. right at the end of the article)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that makes sense.
For simplicity, instead of using eventReactive()
, you could just use isolate()
in the renderPrint()
call, like in the example below. However, looking at the article, I don't see this as one of the 5 patterns there. It is shown in the isolation article though.
@jcheng5, does it seem right to you that the action button article barely talks about using isolate()
? That's the way I almost always use action buttons.
shinyApp(
ui = basicPage(
numericInput("num", label = "Make changes", value = 1),
actionButton("update" ,"Update View", icon("refresh"),
class = "btn btn-primary"),
helpText("When you click the button above, you should see",
"the output below update to reflect the value you",
"entered at the top:"),
verbatimTextOutput("value")
),
server = function(input, output) {
output$value <- renderPrint({
req(input$update)
isolate(input$num)
})
}
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that your version of the example looks simpler. I was using eventReactive()
exactly because it matches up with Pattern 2 in the Action Button article. And I agree, Winston: this article should talk about isolate()
more explicitly. Maybe we could do something like Pattern 2a and Pattern 2b: both delay reactions, but one uses eventReactive()
and the other uses isolate()
... Seems clunky, but for that pattern, I really do think these are equally valid (and the best one depends on the complexity of your app) -- for small things like this, isolate()
is neater than introducing a special reactive for compute the value...
@@ -4,6 +4,54 @@ | |||
#' button do not automatically update their outputs when inputs change, | |||
#' rather they wait until the user explicitly clicks the submit button. | |||
#' | |||
#' @section Warning: | |||
#' Submit buttons are very particular shiny objects. One the weirdest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a trimmed-down version that tells people right off the bat to use actionButtons instead:
Submit buttons are unusual Shiny inputs, and we recommend using \code{\link{actionButton}} instead of \code{submitButton} when you want to delay a reaction. See \href{http://shiny.rstudio.com/articles/action-buttons.html}{this article} for more information.
The presence of a submit button stops all inputs from sending their values automatically to the server, making them unwieldy for all but the simplest apps. If there are \emph{two} submit buttons in the same app, clicking either one will cause all inputs in the app to send their values to the server. Another issue with submit buttons is that dynamically created submit buttons (for example, with \code{\link{renderUI}} or \code{\link{insertUI}}) will not work.
@wch: I followed most of your suggestions. It especially makes sense to tell people right off the bat to use actionButtons. It was my concern with that admonition that made me call the section "Warning" (so people wouldn't skip it!). Also, again, appreciate the conciseness :) I still modified the Details section text a bit from your version to add the info about the "code translation" part of the article (up soon!) and make the second paragraph flow better. I also changed the initial description a bit, to make it clear from the start that we generally recommend action buttons (otherwise, I felt like we were misleading the reader -- I feel like this is too important a piece of info to only introduce it in the Details section). I also changed the wording from "creating a submit button for a form" to "creating a submit button for an app". I feel like we don't actually provide any context for what a form is and most apps that use submit buttons probably don't make their use of the form tag explicit. So it felt like an unnecessary and potentially confusing use of HTML jargon. Let me know if you don't agree with something in here... Finally, do these changes qualify for a NEWS entry? Since we're just essentially updating examples and documentation to follow a pattern we already advocate (and are not really changing anything technical or in the API), I thought it wasn't worth it... But maybe that's the wrong way to think about it, and every PR that changes content (besides fixing typos, etc) should have a NEWS entry..? |
new shiny-dev-center PR: rstudio/shiny-dev-center#33 (includes translation of |
…to use an action button (#1475) * update 07_widgets example * improved documentation for submitButton (including a warnign section and an full-app example) * typo * update documentation based on Winton's feedback
Closes #303 (won't fix -- "submitButton should have a way to specify which inputs it puts on hold"), closes #1355 ("07_widgets example shouldn't use submitButton"), closes #1426 ("Cannot have more than one submit button in an app working correctly").
In addition to the changes here, there will also be related PR to shiny-examples (to update the 07_widgets example to reflect the changes here) and to shiny-dev-center (to update the action button article along these same lines -- mostly highlight the very limited cases when you can use a submit button).