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.

Sign up for GitHub

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

Jump to bottom

parameters with choices and multiple use a select #2576

Merged
merged 3 commits into from
Sep 13, 2024
Merged

parameters with choices and multiple use a select #2576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c01e1b8
parameters with choices and multiple use a select
aronatkins Sep 12, 2024
2d4724c
isTRUE rather than an "is multiple" helper
aronatkins Sep 13, 2024
d23ff28
more thorough explanation of input control inference
aronatkins Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,27 @@ rmarkdown 2.29

- `find_external_resources()` now correctly detects knitr child document provided with option like `child = c("child.Rmd")` (thanks, @rempsyc, #2574).

- `knit_params_ask()` uses a `select` input for parameters which allow multiple selected values. Previously, a `radio` input was incorrectly used when the parameter had a small number of choices.

```yaml
params:
primaries:
choices: ["red", "yellow", "blue"]
multiple: true
```

When `multiple` is not enabled, parameter configuration still uses `radio` when there are fewer than five choices.

The `input` parameter field can still be used to force the configuration control.

```yaml
params:
grade:
input: radio
choices: ["A", "B", "C", "D", "F"]
```


rmarkdown 2.28
================================================================================

Expand Down
13 changes: 7 additions & 6 deletions R/params.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,13 @@ params_get_input <- function(param) {
input <- param$input
if (is.null(input)) {
if (!is.null(param$choices)) {
## radio buttons for a small number of choices, select otherwise.
if (length(param$choices) <= 4) {
input <- "radio"
} else {
## select for a large number of choices and multiple choices.
if (isTRUE(param$multiple)) {
input <- "select"
} else if (length(param$choices) > 4) {
input <- "select"
} else {
input <- "radio"
Comment on lines +164 to +170
Copy link
Member

Choose a reason for hiding this comment

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

For the case of length(choices) > 4 and multiple = FALSE, we should use radio, right?

Suggested change
## select for a large number of choices and multiple choices.
if (isTRUE(param$multiple)) {
input <- "select"
} else if (length(param$choices) > 4) {
input <- "select"
} else {
input <- "radio"
## select for a large number of choices if multiple is not specified
if (is.null(param$multiple))
param$multiple <- length(param$choices) > 4)
input <- if (param$multiple) "select" else "radio"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think we want to continue to use select when there are many choices. Consider a list naming all of the countries in the world. A set of radio buttons would consume way too much real estate.

Radio buttons are reserved for small numbers of choices when you only want to choose one. Using four items as the tipping point felt about right when we were first working on this. Radio buttons work when you've got a "handful" but not more. It's totally subjective, though, and someone can always override that heuristic by declaring input: radio or input: select in their document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also: Your suggestion is setting param$multiple when it is not user-specified. That changes the behavior of select and turns it from a single-selector to a multi-selector.

Many of the parameter fields become arguments to the Shiny UI controls. In this case, we pass multiple to shiny::selectInput().

https://rstudio.github.io/shiny/reference/selectInput.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That last comment probably doesn't apply since the value for param$multiple does not escape this function.

Sorry for the repeated replies on this one... Your comment and code might be inconsistent. We never want to use radio for a large number of inputs. I think your code accomplishes that, but your PR comment (not the code comment) says otherwise.

At any rate -- I think the tests capture the scenarios accurately. If you think your rewrite is worth it, fine. I worry that setting param$multiple like you have is going to confuse the "future us" because we are giving it a TRUE value even when the parameter does not want to allow multiple choices.

}
} else {
## Not choices. Look at the value type to find what input control we
Expand Down Expand Up @@ -219,8 +221,7 @@ params_configurable <- function(param) {
}
# Some inputs (like selectInput) support the selection of
# multiple entries through a "multiple" argument.
multiple_ok <- (!is.null(param$multiple) && param$multiple)
if (multiple_ok) {
if (isTRUE(param$multiple)) {
return(TRUE)
}
# sliderInput supports either one or two-value inputs.
Expand Down
80 changes: 80 additions & 0 deletions tests/testthat/test-params.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,86 @@ test_that("parameters are configurable", {
value = c(10, 20, 30))))
})

test_that("params_get_input", {
# input derived from value type.
expect_equal(params_get_input(list(
value = TRUE
)), "checkbox")
expect_equal(params_get_input(list(
value = as.integer(42)
)), "numeric")
expect_equal(params_get_input(list(
value = 7.5
)), "numeric")
expect_equal(params_get_input(list(
value = "character"
)), "text")
expect_equal(params_get_input(list(
value = Sys.Date()
)), "date")
expect_equal(params_get_input(list(
value = Sys.time()
)), "datetime")

# numeric becomes a slider with both min and max
expect_equal(params_get_input(list(
value = as.integer(42),
min = 0
)), "numeric")
expect_equal(params_get_input(list(
value = as.integer(42),
max = 100
)), "numeric")
expect_equal(params_get_input(list(
value = as.integer(42),
min = 0,
max = 100
)), "slider")
expect_equal(params_get_input(list(
value = 7.5,
min = 0
)), "numeric")
expect_equal(params_get_input(list(
value = 7.5,
max = 10
)), "numeric")
expect_equal(params_get_input(list(
value = 7.5,
min = 0,
max = 10
)), "slider")

# choices implies radio or select
expect_equal(params_get_input(list(
value = "red",
choices = c("red", "green", "blue")
)), "radio")
expect_equal(params_get_input(list(
value = "red",
multiple = TRUE,
choices = c("red", "green", "blue")
)), "select")
expect_equal(params_get_input(list(
value = "red",
choices = c("red", "green", "blue", "yellow", "black", "white")
)), "select")
expect_equal(params_get_input(list(
value = "red",
multiple = TRUE,
choices = c("red", "green", "blue", "yellow", "black", "white")
)), "select")

# explicit input overrides inference
expect_equal(params_get_input(list(
input = "slider",
value = 42
)), "slider")
expect_equal(params_get_input(list(
input = "file",
value = "data.csv"
)), "file")
})

test_that("params hidden w/o show_default", {

# file input is always NULL
Expand Down
Loading