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

Variable Select Input #2091

Merged
merged 10 commits into from Jun 14, 2018
Merged

Variable Select Input #2091

merged 10 commits into from Jun 14, 2018

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Jun 7, 2018

For use within tidy eval.

Kitchen Variable Select Input Example: https://gist.github.com/schloerke/580ee1587e58ac1f367f00bb1acc22fc
Link to app: https://beta.rstudioconnect.com/connect/#/apps/3782/access

Two small examples:

shinyApp(
  ui = fluidPage(
    varSelectInput("variable", "Variable:", mtcars),
    plotOutput("data")
  ),
  server = function(input, output) {
    output$data <- renderPlot({
      ggplot2::qplot(!!input$variable, data = mtcars)
    })
  }
)

shinyApp(
  ui = fluidPage(
    varSelectInput("variable", "Variable:", mtcars, multiple = TRUE),
    tableOutput("data")
  ),
  server = function(input, output) {
    output$data <- renderTable({
       if (length(input$variable) == 0) return(mtcars)
       mtcars %>% dplyr::select(!!!input$variable)
    }, rownames = TRUE)
  }
)

@jskress jskress added the review label Jun 7, 2018
@schloerke schloerke requested a review from hadley June 7, 2018 20:55
@wch
Copy link
Collaborator

wch commented Jun 7, 2018

Do you mind deploying the app to rsc.radixu.com, or beta.rstudioconnect.com?

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Looks good. I don't have any deep insight into the implementation (although the coercions look fine); so I primarily reviewed the docs.

R/input-select.R Outdated

#' Create a select list input control from a data.frame
#'
#' Create a select list that can be used to choose a single or multiple items
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a duplicate of the title? I think you can omit

R/input-select.R Outdated



#' Create a select list input control from a data.frame
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "Select variables from a data frame" ?

R/input-select.R Outdated
#' Create a select list that can be used to choose a single or multiple items
#' from the column names of a data frame.
#'
#' The resulting output value will be returned as a symbol or a list of symbols
Copy link
Member

Choose a reason for hiding this comment

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

... returned as a symbol (if multiple = FALSE) or a list of symbols (if multiple = TRUE)...

Or maybe make a bulleted list? Then you could explain !! vs !!! for each bullet too.

R/input-select.R Outdated
#' ),
#' server = function(input, output) {
#' output$data <- renderPlot({
#' ggplot2::qplot(!!input$variable, data = mtcars)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid qplot() if possible.

@schloerke
Copy link
Collaborator Author

Thanks @hadley for the docs check! (updated details, title, and changed qplot to ggplot)

R/input-select.R Outdated
)

# set the select tag class to be "symbol"
selectAttribs <- selectInputVal$children[[2]]$children[[1]]$attribs
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a check here that selectInputVal$children[[2]]$children[[1]] is actually a <select>? (Either here, or in a unit test--and if a unit test, both selectize=TRUE and FALSE.) I just want to be sure we catch it if we slightly change the DOM structure of selectInput.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add a unit test

@@ -90,8 +90,9 @@ Suggests:
URL: http://shiny.rstudio.com
BugReports: https://github.com/rstudio/shiny/issues
Remotes:
tidyverse/ggplot2,
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we wouldn't be able to release Shiny to CRAN without a ggplot2 release first? If so, any idea of a timeline @hadley?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to work with the aes() I believe we'll have to wait. But that's only for the example.

If there is a different example, we could use it instead to release sooner.

Copy link
Member

Choose a reason for hiding this comment

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

We're in the middle of the release process, with submission scheduled for June 25.

Copy link
Member

@hadley hadley Jun 8, 2018

Choose a reason for hiding this comment

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

(But equally it would be fine to eliminate that example — you can't replace it with anything particularly useful in CRAN ggplot2)

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

5 participants