Fix multiselect in ComboBox #161

jakubsob · 2023-02-27T09:07:04Z

Changes

Closes #160

How to test

library(shiny)
library(shiny.fluent)

shinyApp(
  ui = fluentPage(
    ComboBox.shinyInput(
      inputId = "multiselect",
      multiSelect = TRUE,
      options = list(
        list(key = "A", text = "A"),
        list(key = "B", text = "B"),
        list(key = "C", text = "C")
      )
    ),
    ComboBox.shinyInput(
      inputId = "singleselect",
      multiSelect = FALSE,
      options = list(
        list(key = "A", text = "A"),
        list(key = "B", text = "B"),
        list(key = "C", text = "C")
      )
    )
  ),
  server = function(input, output, session) {

  }
)

kamilzyla

It works, thanks! 😁 🎉

Please run yarn webpack and commit the compiled JS.

kamilzyla · 2023-03-28T12:40:01Z

js/src/inputs.jsx

+      newValue = option.selected
+        ? [...newValue, option.key]
+        : newValue.filter((key) => key !== option.key);
+      setValue(newValue);


The code in this if branch is very similar to the code in Dropdown below. Perhaps we can make it a function? (Arguments: props.options, option; return newValue).

jakubsob · 2023-04-24T09:51:22Z

Additional test app to also test Dropdown

library(shiny)
library(shiny.fluent)

options <- list(
  list(key = "A", text = "Option A"),
  list(key = "B", text = "Option B"),
  list(key = "C", text = "Option C")
)

ui <- function(id) {
  ns <- NS(id)
  div(
    ComboBox.shinyInput(
      ns("combo"),
      options = options,
      multiSelect = TRUE,
      value = "A"
    ),
    textOutput(ns("comboValue")),
    Dropdown.shinyInput(
      ns("dropdown"),
      value = "A",
      options = options,
      multiSelect = TRUE
    ),
    textOutput(ns("dropdownValue"))
  )
}

server <- function(id) {
  moduleServer(id, function(input, output, session) {
    output$comboValue <- renderText({
      dput(input$combo)
    })
    output$dropdownValue <- renderText({
      input$dropdown
    })
  })
}

kamilzyla

It's been a long time! 😅 Thanks for the changes 💙 I did a bit more testing and discovered some issues. I used the following app for testing:

library(shiny)
library(shiny.fluent)

options <- list(
  list(key = "A", text = "Apples"),
  list(key = "B", text = "Bananas"),
  list(key = "C", text = "Cherries")
)

ui <- div(
  textOutput("comboValue"),
  ComboBox.shinyInput("combo",
    options = options,
    allowFreeform = TRUE
  ),
  textOutput("dropdownValue"),
  Dropdown.shinyInput("dropdown",
    options = options
    # multiSelect = TRUE
  )
)

server <- function(input, output, session) {
  output$comboValue <- renderText(paste("ComboBox:", deparse(input$combo)))
  output$dropdownValue <- renderText(paste("Dropdown:", deparse(input$dropdown)))
}

shinyApp(ui, server)

Running `yarn webpack` shows changes

Did you build and commit the latest JS? I did my remaining testing with the changes I got after running yarn webpack.

Initial value is not properly registered

Steps to reproduce:

Click apples in the ComboBox. The checkbox does not check, but the value becomes "A".
Click apples again. The checkbox checks and the value becomes c("A", "A").

Expected behavior: The checkbox checks upon the first click and value becomes "A". The second click unchecks the checkbox and the value becomes NULL again.

There are some variations to this issue:

Clicking apples and then bananas will lead to only bananas being checked, but the value will be c("A", "B").
Passing value = “A” seems to do the same thing as step (1), i.e. the behavior is as if apples were clicked.

Freeform input doesn’t work with multiselect

Steps to reproduce:

Uncomment the allowFreeform = TRUE argument.
Type any text into the ComboBox and press enter.
The value remains NULL. Additional quirk: the text inside the ComboBox disappears when it loses focus and reappears when it is focused again.

Expected behavior: Good question. To get a better idea of a reasonable API, let’s first see how the underlying component works with freeform & multiselect:

ComboBox(
  options = options,
  allowFreeform = TRUE,
  multiSelect = TRUE
)

I’m not sure what would be the best way to represent a selection of choices, where some come from the predefined options and some are entered with freeform. One possibility would be to have c(text = “Whatever”, key = “a”, text = “you want”), but I see many alternatives.

For now I would raise an error in R if allowFreeform is used together with multiSelect, and create a separate issue for this use case.

erikrenz88 · 2023-09-11T17:49:55Z

Still seeing the issue from #160? Wondering if this has been paused?

averissimo

It looks good overall. I've tested multiple scenarios and couldn't find a problem.

Great job

I do have some comments about tests:

Could you re-add the tests that were removed?
- at least running with default arguments (as diff below does)
Add a test to make sure deselecting works
- code is also included below

note: it seems that most tests are running sequentially, which might be problematic :-\

diff --git a/js/cypress/integration/e2e-test/fluentComponents.js b/js/cypress/integration/e2e-test/fluentComponents.js
index a7bb51b..0e5715d 100644
--- a/js/cypress/integration/e2e-test/fluentComponents.js
+++ b/js/cypress/integration/e2e-test/fluentComponents.js
@@ -183,6 +183,29 @@ function dropdownChangeTest() {
   cy.get('#fluentInputs-dropdownValue').contains('Value: C');
 }
 
+function dropdownMultiselectDefaultTest(value = 'Option A, Option C', output = 'Value: A Value: C') {
+  cy.get('#fluentInputs-dropdownMultiselect').within(() => {
+    cy.get('#fluentInputs-dropdownMultiselect-option').should('contain', `${value}`);
+  });
+  cy.get('#fluentInputs-dropdownMultiselectValue').should('contain', `${output}`);
+}
+
+function dropdownMultiselectChangeTest() {
+  cy.get('#fluentInputs-dropdownMultiselect-option').click();
+  cy.get('#fluentInputs-dropdownMultiselect-list0').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-list1').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-option').should('contain', 'Option C, Option B');
+  cy.get('#fluentInputs-dropdownMultiselectValue').contains('Value: C Value: B');
+}
+
+function dropdownMultiselectDeselectTest() {
+  cy.get('#fluentInputs-dropdownMultiselect-option').click();
+  cy.get('#fluentInputs-dropdownMultiselect-list0').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-list2').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-option').invoke('text').should("be.empty");
+  cy.get('#fluentInputs-dropdownMultiselectValue').invoke('text').should("be.empty");
+}
+
 function datePickerDefaultTest(date = 'Thu Jun 25 2020', dttm = '2020-06-25T12:00:00.000Z') {
   cy.get('#fluentInputs-datePicker-label').should('have.attr', 'value', date);
   cy.get('#fluentInputs-datePickerValue').should('contain', `Value: ${dttm}`);
@@ -335,6 +358,10 @@ describe('ComboBox.shinyInput()', () => {
 });
 
 describe('Dropdown.shinyInput()', () => {
+  beforeEach(() => {
+    cy.reload();
+  })
+
   it('setting default values works', () => {
     dropdownDefaultTest();
   });
@@ -342,6 +369,18 @@ describe('Dropdown.shinyInput()', () => {
   it('value change works', () => {
     dropdownChangeTest();
   });
+
+  it('setting default values for multiSelect works', () => {
+    dropdownMultiselectDefaultTest();
+  });
+
+  it('updating multiSelect options and values works', () => {
+    dropdownMultiselectChangeTest();
+  });
+
+  it('updating multiSelect by deselecting all options and values works', () => {
+    dropdownMultiselectDeselectTest();
+  });
 });
 
 describe('DatePicker.shinyInput()', () => {

averissimo · 2023-09-18T09:51:00Z

js/cypress/integration/e2e-test/fluentComponents.js

-  it('setting default values for multiSelect works', () => {
-    dropdownMultiselectDefaultTest();
-  });
-
-  it('updating multiSelect options and values works', () => {
-    dropdownMultiselectChangeTest();
-  });
 });


Why did you remove these tests? They work as intended

Well, I started observing some intermittency in those tests, which I don't quite understand. It seems that now it takes a bit longer to spin up the test app and that's why some fail because components are not yet rendered. For this multiselect when I run it interactively everything works fine, but on the whole Cypress run it tends to fail -- I have big troubles debugging it.

I'd prefer to refactor those tests to be more isolated, as all components being put in one big app seems to be the root cause of this intermittency. But for now I'm a bit stuck with this test, I'll give it one more try

I'm confused @averissimo… Why it passed now when if didn't on multiple runs previously?

I restored multiselect tests, but since it required some changes in shiny.react, here is a PR that enables this fix, PTAL.

I agree with refactoring. But that's out of scope for this PR 😁

The main issue I found was that it was running the test sequentially and keeping the state between tests. (hence the beforeEach(() => cy.reload()) I had on my diff.

I've approved the Appsilon/shiny.react#75

averissimo

I like the solution!! I have 4 comments:

I think the inst/examples/ComboBox.R example needs a small fix (if the list(text = "...") is not supported. (see diff below)
- Are we dropping that support for defaultValue = list(text = "...")
Should the default value be appended to the list of options?
The allowFreeForm = TRUE is not updating the props.options as new values are added
- edit: Which is weird as if we log the calls to ComboBox the props.options are updated (see screenshot below)

diff --git a/inst/examples/ComboBox.R b/inst/examples/ComboBox.R
index d9212d3..fe3a181 100644
--- a/inst/examples/ComboBox.R
+++ b/inst/examples/ComboBox.R
@@ -10,7 +10,7 @@ options <- list(
 ui <- function(id) {
   ns <- NS(id)
   div(
-    ComboBox.shinyInput(ns("combo"), value = list(text = "some text"),
+    ComboBox.shinyInput(ns("combo"), value = "some text",
       options = options, allowFreeform = TRUE
     ),
     textOutput(ns("comboValue"))
@@ -20,7 +20,7 @@ ui <- function(id) {
 server <- function(id) {
   moduleServer(id, function(input, output, session) {
     output$comboValue <- renderText({
-      sprintf("Value: %s", input$combo$text)
+      sprintf("Value: %s", input$combo)
     })
   })
 }

Screenshot (see the list of options showing in the browser)

averissimo · 2023-11-13T11:37:51Z

js/src/inputs.jsx

+    selectedKey: value,
+    text: getSelectedOptionText(options, value, props.multiSelectDelimiter || ', ') || value,


Should we keep the list(text = "some text") parameters?

Suggested change

selectedKey: value,

text: getSelectedOptionText(options, value, props.multiSelectDelimiter || ', ') || value,

selectedKey: value?.key || value?.text || value,

text: getSelectedOptionText(options, value, props.multiSelectDelimiter || ', ') || value?.text || value,

averissimo · 2023-11-13T13:02:43Z

@jakubsob Can you replicate this bug? Where it takes the "Option A" as the key starts with "A"

inst/examples/ComboBox.R

Screencast.from.2023-11-13.14-01-10.webm

jakubsob changed the title ~~fix: multiselect in ComboBox~~ Fix multiselect in ComboBox Mar 27, 2023

jakubsob requested a review from kamilzyla March 27, 2023 08:08

kamilzyla requested changes Mar 28, 2023

View reviewed changes

kamilzyla requested changes Jun 15, 2023

View reviewed changes

jakubsob force-pushed the fix/160-multiselect-combobox branch from 3f11c13 to 9715d1c Compare June 29, 2023 11:56

jakubsob requested review from kamilzyla and removed request for kamilzyla September 12, 2023 11:46

Jakub Sobolewski added 4 commits September 12, 2023 16:02

fix: multiselect in ComboBox

dd4e66b

refactor: extract multiselect logic to function

a89d632

fix: first selection in ComboBox

90bb3bc

chore: build js

0ee96aa

jakubsob force-pushed the fix/160-multiselect-combobox branch from 9715d1c to 0ee96aa Compare September 12, 2023 14:04

Jakub Sobolewski added 5 commits September 13, 2023 10:14

fix: default value in ComboBox

d4412b1

chore: update browserslist

fa81192

feat: support freeform in multiselect ComboBox

6608a73

feat: select freeform option by default in multiselect ComboBox

52fb740

chore: build js

d0fcec9

jakubsob force-pushed the fix/160-multiselect-combobox branch 4 times, most recently from 3548dee to 310cfaf Compare September 14, 2023 07:29

chore: install shiny.react from remote

7107624

jakubsob force-pushed the fix/160-multiselect-combobox branch from 310cfaf to 7107624 Compare September 14, 2023 07:29

Jakub Sobolewski added 5 commits September 14, 2023 14:24

fix: e2e tests app

ddea2ab

fix: display text after server side update

29796a8

chore: build js

6cadc1a

feat: install shiny.react in ci

5b31b1c

feat: add github token to install shiny.react from github

e6ca47c

Jakub Sobolewski added 3 commits September 14, 2023 15:33

fix: js linter errors

3308cbf

chore: update ci

3ea5a50

fix: ci

914518c

jakubsob force-pushed the fix/160-multiselect-combobox branch from 8152eb2 to 914518c Compare September 15, 2023 14:00

jakubsob assigned averissimo Sep 15, 2023

averissimo reviewed Sep 18, 2023

View reviewed changes

refactor: restore e2e test for multiselect dropdown

f43d10c

jakubsob mentioned this pull request Oct 11, 2023

Allow overriding props in InputAdapter Appsilon/shiny.react#75

Open

jakubsob requested a review from averissimo October 30, 2023 08:43

averissimo requested changes Nov 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiselect in ComboBox #161

Fix multiselect in ComboBox #161

jakubsob commented Feb 27, 2023

kamilzyla left a comment

kamilzyla Mar 28, 2023

jakubsob commented Apr 24, 2023

kamilzyla left a comment •

edited

erikrenz88 commented Sep 11, 2023

averissimo left a comment •

edited

averissimo Sep 18, 2023

jakubsob Oct 10, 2023

jakubsob Oct 11, 2023

averissimo Nov 13, 2023 •

edited

averissimo left a comment •

edited

averissimo Nov 13, 2023

averissimo commented Nov 13, 2023

		selectedKey: value,
		text: getSelectedOptionText(options, value, props.multiSelectDelimiter \|\| ', ') \|\| value,

Fix multiselect in ComboBox #161

Are you sure you want to change the base?

Fix multiselect in ComboBox #161

Conversation

jakubsob commented Feb 27, 2023

Changes

How to test

kamilzyla left a comment

Choose a reason for hiding this comment

kamilzyla Mar 28, 2023

Choose a reason for hiding this comment

jakubsob commented Apr 24, 2023

kamilzyla left a comment • edited

Choose a reason for hiding this comment

Running yarn webpack shows changes

Initial value is not properly registered

Freeform input doesn’t work with multiselect

erikrenz88 commented Sep 11, 2023

averissimo left a comment • edited

Choose a reason for hiding this comment

averissimo Sep 18, 2023

Choose a reason for hiding this comment

jakubsob Oct 10, 2023

Choose a reason for hiding this comment

jakubsob Oct 11, 2023

Choose a reason for hiding this comment

averissimo Nov 13, 2023 • edited

Choose a reason for hiding this comment

averissimo left a comment • edited

Choose a reason for hiding this comment

Screenshot (see the list of options showing in the browser)

averissimo Nov 13, 2023

Choose a reason for hiding this comment

averissimo commented Nov 13, 2023

kamilzyla left a comment •

edited

Running `yarn webpack` shows changes

averissimo left a comment •

edited

averissimo Nov 13, 2023 •

edited

averissimo left a comment •

edited