-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
textInput()
, textAreaInput()
, numericInput()
, passwordInput()
: allow updating value on blur
#4183
base: main
Are you sure you want to change the base?
Conversation
textInput()
: allow updating value on blur and allow setting debounce delaytextInput()
: allow updating value on blur
textInput()
: allow updating value on blurtextInput()
, numericInput()
, passwordInput()
: allow updating value on blur
textInput()
, numericInput()
, passwordInput()
: allow updating value on blurtextInput()
, textAreaInput()
, numericInput()
, passwordInput()
: allow updating value on blur
srcts/src/bindings/input/text.ts
Outdated
$el.on( | ||
"change.textInputBinding", | ||
// event: Event | ||
function () { | ||
callback(false); | ||
} | ||
); |
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.
For NumberInputBinding
, this change
event can get triggered:
- programmatically, via
updateNumericInput()
on the server - by clicking on the up/down arrows, or pressing up/down keys on the keyboard.
When we enable the option, we'll want the callback to be invoked when receiving the server message, but not when the up/down arrow event causes it.
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 handled this in 0f0e4e5 by adding extra data to the change event emitted by the .receiveMessage()
method, whose presence signals that the origin of the event was a server-side message. Then, when updateOn === "blur"
, we ignore change events that aren't server driven.
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.
One thing that's worth considering is the possibility that someone would trigger a change
event on a programmatically via JS, like if they use shinyjs. Requiring fromServer: true
could make that stop working.
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.
Good point; I'll think about this some more
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.
Oh! I came up with an elegant solution (in d322976): rather than using the event data, we'll just ignore any change events that happen when the element has focus. It's unlikely that programmatic changes will happen while an element has focus, so in most cases the update is immediate. But if the changed input currently has focus, we'll just update on blur like we otherwise would.
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.
Oh great! That’s much nicer than the other solutions.
@wch This PR is ready to go! I've implemented all the changes we discussed. Here's a small app based on your example that shows the difference between Could you give this PR a quick review? Example applibrary(shiny)
text_input_ui <- function(updateOn = "change") {
ns <- NS(updateOn)
tagList(
h2(sprintf('updateOn="%s"', updateOn)),
textInput(ns("txt"), "Text", "Hello", updateOn = updateOn),
textAreaInput(ns("txtarea"), "Text Area", updateOn = updateOn),
numericInput(ns("num"), "Numeric", 1, updateOn = updateOn),
passwordInput(ns("pwd"), "Password", updateOn = updateOn),
verbatimTextOutput(ns("value")),
actionButton(ns("update_text"), "Update Text"),
actionButton(ns("update_text_area"), "Update Text Area"),
actionButton(ns("update_number"), "Update Number"),
actionButton(ns("update_pwd"), "Update Password"),
)
}
text_input_server <- function(id) {
moduleServer(id, function(input, output, session) {
output$value <- renderText({
paste(
"---- Text ----",
input$txt,
"---- Text Area ----",
input$txtarea,
"---- Numeric ----",
input$num,
"---- Password ----",
input$pwd,
sep = "\n"
)
})
observeEvent(input$update_number, {
updateNumericInput(session, "num", value = floor(runif(1, 0, 100)))
})
observeEvent(input$update_text_area, {
updateTextAreaInput(
session,
"txtarea",
value = paste(sample(LETTERS, 5), collapse = "\n")
)
})
observeEvent(input$update_text, {
updateTextInput(
session,
"txt",
value = paste(sample(letters, 12), collapse = " ")
)
})
observeEvent(input$update_pwd, {
updateTextInput(
session,
"pwd",
value = paste(sample(letters, 8), collapse = "")
)
})
})
}
ui <- fluidPage(
fluidRow(
column(6, class = "col-sm-12", text_input_ui("change")),
column(6, class = "col-sm-12", text_input_ui("blur"))
)
)
server <- function(input, output, session) {
text_input_server("change")
text_input_server("blur")
}
shinyApp(ui, server)
|
#' update the input immediately whenever the value changes. Use `"blur"`to | ||
#' delay the input update until the input loses focus (the user moves away | ||
#' from the input), or when Enter is pressed (or Cmd/Ctrl + Enter for | ||
#' [textAreaInput()]). |
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 wonder if some people will want to have a third option, "enter"
? In that scenario, we'd allow multiple values for updateOn
, so you could say updateOn = c("blur", "enter")
.
That said, I prefer the implementation as it is now; I don't think in practice in Shiny apps it's a great idea to require pressing Enter in these inputs for the value to change, or if you do you should use an button (or we extract the shinychat text input into a standalone component).
This PR updates
textInput()
,textAreaInput()
,numericInput()
, andpasswordInput()
, so that:updateOn="blur"
is used, the input value will be updated only when the user presses Enter, or the input loses focus.Example app:
Notes:
textAreaInput()
, it only updates on blur; when the user presses Enter, it adds a newline character. Maybe it should also update when the user presses ctrl- or cmd-Enter?keydown
event handler forEnter
orCmd/Ctrl + Enter
(textarea inputs), rather than relying on thechange
event.numericInput()
, the value still updates when the up/down arrow are pressed. I think that's probably because those keypresses trigger achange
event.