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

parameters with choices and multiple use a select #2576

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

aronatkins
Copy link
Contributor

a small number of choices does not force radio buttons.

A document defined like:

---
title: selects
params:
    multifewchoices:
        multiple: true
        choices: ["boston","springfield","worcester"]
        value: "boston"
---

Running rmarkdown::knit_params_ask("inputs.Rmd") presents the UI:

image

This previously required an explicit input declaration:

---
title: selects
params:
    multifewchoices:
        input: multiple
        multiple: true
        choices: ["boston","springfield","worcester"]
        value: "boston"
---
image

With this change, the input is unnecessary.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Thanks!

R/params.R Outdated Show resolved Hide resolved
NEWS.md Outdated
@@ -1,6 +1,7 @@
rmarkdown 2.29
================================================================================

- A parameter allowing multiple selections uses a select input control by default regardless of the number of choices.
Copy link
Member

Choose a reason for hiding this comment

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

Users probably have no idea what this short sentence means.

Suggested change
- A parameter allowing multiple selections uses a select input control by default regardless of the number of choices.
- Added an option `multiple` for parameters with `choices` to use the `select` or `radio` input control regardless of the number of choices. Previously, the `select` input is used when the number of choices is greater than 4, otherwise the `radio` input is used. Now `select` will be used when `multiple` is true, and `radio` is used when `multiple` is false, e.g.,
---
params:
foo:
choices: ["a", "b", "c"]
multiple: true # use select input
bar:
choices: ["a", "b", "c", "d", "e"]
multiple: false # use radio input
---
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have long supported "multiple". Previously, we made the wrong choice when there were a small number of choices and multiple were allowed.

This is the one test case that demonstrates the change:

  expect_equal(params_get_input(list(
    value = "red",
    multiple = TRUE,
    choices = c("red", "green", "blue")
  )), "select")

Previously, the small number of choices causes us to inferred a "radio" input without recognizing the multiple = TRUE constraint.

I've pushed a NEWS update, but it feels too wordy for such a small, subtle change. I defer to your judgement.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Thanks for the explanation! I have totally forgotten the multiple option.

@aronatkins aronatkins force-pushed the aron-params-multiple-choices-select branch from 343e71b to d23ff28 Compare September 13, 2024 12:46
@aronatkins
Copy link
Contributor Author

(force-pushed after rebasing from main and resolving NEWS.md conflicts)

Comment on lines +164 to +170
## 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"
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.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

I see what you mean now. Thanks again for the explanation!

@yihui yihui merged commit c787e8a into main Sep 13, 2024
19 checks passed
@yihui yihui deleted the aron-params-multiple-choices-select branch September 13, 2024 21:06
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Oct 17, 2024
Merge remote-tracking branch 'rstudio/main' into jg-devel

# By Yihui Xie (4) and others
# Via GitHub
* rstudio/main:
  parameters with `choices` and `multiple = TRUE` should use a `select` input (rstudio#2576)
  Add support for `child=c("child.Rmd")` in find_external_resources() (rstudio#2575)
  Support new Pandoc 3.4 behavior with GFM math (rstudio#2573)
  consistent dev install instructions (rstudio#2571)
  Update actions/checkout to v4 (rstudio#2570)
  start the next version
  CRAN release v2.28
  update tests for lua filters accordingly
  fix rstudio#2567: add classes `odd`, `even`, and `header` back to table rows for Pandoc >= 3.2.1

# Conflicts:
#	DESCRIPTION
#	NEWS.md
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Oct 17, 2024
* jg-devel:
  parameters with `choices` and `multiple = TRUE` should use a `select` input (rstudio#2576)
  Add support for `child=c("child.Rmd")` in find_external_resources() (rstudio#2575)
  Support new Pandoc 3.4 behavior with GFM math (rstudio#2573)
  consistent dev install instructions (rstudio#2571)
  Update actions/checkout to v4 (rstudio#2570)
  start the next version
  CRAN release v2.28
  update tests for lua filters accordingly
  fix rstudio#2567: add classes `odd`, `even`, and `header` back to table rows for Pandoc >= 3.2.1
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Oct 17, 2024
* rstudio-main:
  parameters with `choices` and `multiple = TRUE` should use a `select` input (rstudio#2576)
  Add support for `child=c("child.Rmd")` in find_external_resources() (rstudio#2575)
  Support new Pandoc 3.4 behavior with GFM math (rstudio#2573)
  consistent dev install instructions (rstudio#2571)
  Update actions/checkout to v4 (rstudio#2570)
  start the next version
  CRAN release v2.28
  update tests for lua filters accordingly
  fix rstudio#2567: add classes `odd`, `even`, and `header` back to table rows for Pandoc >= 3.2.1
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.

2 participants