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
Maintain names of factors when updating radio buttons' choices #1397
Conversation
After talking to Winston, I went back and fixed the original problem (#1093) better. This had been solved in #1291, but introduced the bug (#1396) that I was fixing now. The better way does the casting to character directly in the I had to chance tests, but everything is working as expected. Here's an example app to make sure this works: library(shiny)
ui <- fluidPage(
radioButtons("rd", "radio buttons",
c("Item A"=1, "Item B"=2, "Item C"=3)),
selectInput("slct", "Select input",
c("Item A"=1, "Item B"=2, "Item C"=3)),
checkboxGroupInput("chk", "Select input",
c("Item A"=1, "Item B"=2, "Item C"=3)),
actionButton("update","update"),
textOutput("selected")
)
server <- function(input, output, session) {
observeEvent(input$update, {
updateRadioButtons(session, "rd",
choices = c("Item X"=1, "Item Y"=2, "Item Z"=3))
updateSelectInput(session, "slct",
choices = c("Item X"=1, "Item Y"=2, "Item Z"=3))
updateCheckboxGroupInput(session, "chk",
choices = c("Item X"=1, "Item Y"=2, "Item Z"=3))
})
output$selected <- renderText(paste("Selected:", input$RadioButtons))
}
shinyApp(ui, server) |
Could you add a test with nested lists and numbers, like: choicesWithNames(list(A="a", B="b", C=c(D=1))) Also, please add a NEWS entry. |
@@ -7,7 +7,7 @@ controlLabel <- function(controlName, label) { | |||
# refers to values. Below is a function for backward compatibility. |
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.
Can you add a comment to the top of this function saying it always coerces to character. And same for choicesWithNames
(but for leaf nodes there).
@@ -7,7 +7,7 @@ controlLabel <- function(controlName, label) { | |||
# refers to values. Below is a function for backward compatibility. | |||
validateSelected <- function(selected, choices, inputId) { | |||
# drop names, otherwise toJSON() keeps them too | |||
selected <- unname(selected) | |||
selected <- as.character(unname(selected)) |
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.
unname()
isn't actually needed here. Maybe just use as.character()
and update the comment to mention that as.character()
does two things for us here: coerces type, and drops name.
Similar test app, but setting library(shiny)
ui <- fluidPage(
radioButtons("rd", "radio buttons",
c("Item A"=1, "Item B"=2, "Item C"=3)),
selectInput("slct", "Select input",
c("Item A"=1, "Item B"=2, "Item C"=3)),
checkboxGroupInput("chk", "Checkbox group",
c("Item A"=1, "Item B"=2, "Item C"=3)),
actionButton("update","update"),
textOutput("selected")
)
server <- function(input, output, session) {
observeEvent(input$update, {
updateRadioButtons(session, "rd",
selected = 3)
updateSelectInput(session, "slct",
selected = 3)
updateCheckboxGroupInput(session, "chk",
selected = 3)
})
output$selected <- renderText(paste("Selected:", input$rd))
}
shinyApp(ui, server) |
7043286
to
72838c2
Compare
@@ -9,6 +9,8 @@ shiny 0.14.0.9000 | |||
|
|||
### Bug fixes | |||
|
|||
* Fixed [#1093](https://github.com/rstudio/shiny/issues/1093) better: `updateRadioButtons()` and `updateCheckboxGroupInput()` were not working correctly if the choices were given as a numeric vector. This had been solved in [#1291](https://github.com/rstudio/shiny/pull/1291), but that introduced a different bug [#1396](https://github.com/rstudio/shiny/issues/1396) that this better fix avoids. |
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.
This should also have a reference to this PR at the end (1397).
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.
No problem, I just thought we weren't doing that, since only a handful of NEWS entries actually do that. I'll take it this is our new protocol then. I guess it makes sense to do that form now on.
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 yeah, I started doing that and just assumed it should be the standard for all news items. Sorry for not consulting about that. It really is a good idea though. :)
@wch I think this is finally done! I tested a few apps from the Gallery like you suggested and they're working fine. Thanks for the help! |
Fixes #1396