Skip to content
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

Allow arbitrary UI code in the choices for radio buttons and checkbox group input #1521

Merged
merged 22 commits into from Mar 27, 2017

Conversation

bborgesr
Copy link
Contributor

@bborgesr bborgesr commented Dec 30, 2016

Example app:

ui <- fluidPage(
  sidebarLayout(
    sidebarPanel( 
      radioButtons("normal", "Normal radio buttons:",
        c("Normal" = "norm", "Uniform" = "unif",
         "Log-normal" = "lnorm", "Exponential" = "exp")),
      hr(),
      radioButtons("fancy", "Fancy radio buttons:",
      choiceNames = list(tags$span(HTML("Normal &mu;<sub>1</sub>")),
        icon("calendar", "fa-3x"),
        icon("bed"),
        icon("cog", lib = "glyphicon"),
        img(src = "https://www.rstudio.com/wp-content/uploads/2016/09/RStudio-Logo-Blue-Gray-250.png")),
      choiceValues = c("html", "icon1", "icon2", "icon3", "img"))),
    mainPanel(textOutput("rb1"), textOutput("rb2"))
  )
)

server <- function(input, output, session) {
  output$rb1 <- renderText({
    paste("You chose", input$normal)
  })
  
  output$rb2 <- renderText({
    paste("You chose", input$fancy)
  })
}

shinyApp(ui, server)

@bborgesr
Copy link
Contributor Author

bborgesr commented Jan 1, 2017

Fixes #1348 and fixes #1437

@bborgesr bborgesr self-assigned this Jan 1, 2017
@jcheng5
Copy link
Member

jcheng5 commented Jan 1, 2017

I'd prefer choiceNames and choiceValues instead of choicesNames and choicesValues.

I don't like that choices, choicesNames, and choicesValues propagate beyond checkChoicesArgs; I think choices should go away, since it can be thought of as a shorthand for choicesNames <- names(choices) and choicesValues <- unname(choices). Then the rest of the code (like validateSelected) can just deal with one way of specifying these values, instead of two.

I tend to think of functions that are named checkFoo and validateFoo as simply returning TRUE or FALSE, or throwing warnings or errors. In these cases we're using them to do normalization. Not a big deal and you don't have to change the names in this PR but maybe something to keep in mind moving forward.

(Don't forget the inspecting/merging of the API branch needs to happen before the conference, this PR can be merged after the conference...)

R/input-utils.R Outdated
stop("`choicesNames` and `choicesValues` must not be named.")
}
} else {
if (lenNames != 0 || lenVals != 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for NULL instead

R/input-utils.R Outdated
if (lenNames != lenVals) {
stop("`choicesNames` and `choicesValues` must have the same length.")
}
if (!is.null(names(choicesNames)) || !is.null(names(choicesValues))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use anyNamed instead

R/input-utils.R Outdated
if (lenNames != 0 || lenVals != 0) {
warning("Using `choices` argument; ignoring `choicesNames` and
`choicesValues`.")
choicesNames = NULL
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be <-.

@bborgesr bborgesr force-pushed the barbara/htmlChoices branch 2 times, most recently from d8671c3 to 057e404 Compare March 22, 2017 02:34
@bborgesr
Copy link
Contributor Author

I think I've addressed all points and this should be good to squash-merge.

The one possible hiccup is what we're breaking backward compatibility (but only for unexported functions!) as we change the name of some of the internal utility functions (switch from "validate"/"check" prefixes to "normalize", per Joe's comment above) and their expected arguments (pass in choiceNames and choiceValues instead of choices).

@jcheng5
Copy link
Member

jcheng5 commented Mar 23, 2017

Maybe we can review this together tomorrow. It's been so long I don't remember :)

R/input-utils.R Outdated

# Before shiny 0.9, `selected` refers to names/labels of `choices`; now it
# refers to values. Below is a function for backward compatibility. It also
# coerces the value to `character`.
validateSelected <- function(selected, choices, inputId) {
normalizeSelected <- function(selected, inputId, choiceNames, choiceValues) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since 0.10.0 (which was released in June 2014) users have gotten a warning if selected was a name instead of a value. So I think it's safe to assume that no one is doing that anymore. And that means that we can remove this function entirely. We will need to do selected <- as.character(selected) where we previously called validateSelected, for the reasons in the comment directly below.

R/update-input.R Outdated
type = 'checkbox') {
if (!is.null(choices))
choices <- choicesWithNames(choices)
selected = NULL, inline = FALSE, type = NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to error if type is NULL.

R/input-utils.R Outdated
choiceValues <- unname(choices)
}

return(list(choiceNames = choiceNames, choiceValues = choiceValues))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If choices, choicesNames, and choicesValues are all NULL, this should return a list like:

list(choiceNames = NULL, choiceValues = NULL)

And also have a test for it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, choiceNames and choiceValues should always be lists (or NULL), and not character vectors.

NEWS.md Outdated
@@ -5,6 +5,8 @@ shiny 1.0.0.9001

### Breaking changes

* The functions `radioButtons()`, `checkboxGroupInput()` and `selectInput()` (and the corresponding `updateXXX()` functions) no longer accept a `selected` argument whose value is the name of a choice, instead of the value of the choice. This feature had been soft-deprecated since Shiny 0.10 (printing a warning message, but still trying to match the name to the right choice) and it's now been completely deprecated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more accurate to say that it had been deprecated (not soft-deprecated) since 0.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants