Fix broken draggable_buckets

[vedhav]

Closes #308

The subscribe callback now expects an object with the priority after this PR rstudio/shiny#4211. So, if you update to shiny v1.11.1 the draggable_buckets do not work (the init works, but since the subscribe is broken it does not update).

In the future we can think about using sortable package to replace a huge chunk of custom css/js code.

[github-actions]

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  --------------------------------------------------
R/basic_table_args.R             23       0  100.00%
R/draggable_buckets.R            87      87  0.00%    4-163
R/get_dt_rows.R                  13      13  0.00%    44-56
R/ggplot2_args.R                 49       0  100.00%
R/nested_closeable_modal.R       16      16  0.00%    83-98
R/optionalInput.R               255     212  16.86%   140-438, 501, 562, 568, 583-596
R/panel_group.R                  39      39  0.00%    50-136
R/plot_with_settings.R          309      16  94.82%   299-305, 327, 364, 373-374, 390, 578-579, 581, 583
R/standard_layout.R              52      20  61.54%   81-99, 106
R/table_with_settings.R         158       1  99.37%   100
R/utils.R                         7       0  100.00%
R/verbatim_popup.R              107      52  51.40%   75-90, 116-117, 119, 127-158, 179
R/white_small_well.R              7       7  0.00%    19-25
TOTAL                          1122     463  58.73%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 484ef4f

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   16 suites   1m 52s ⏱️
124 tests 124 ✅ 0 💤 0 ❌
327 runs  327 ✅ 0 💤 0 ❌

Results for commit 484ef4f.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
draggable_buckets	👶		$+8.67$	$+5$	$0$	$0$	$0$
draggable_buckets_ui	💀	$3.25$	$-3.25$	$-2$	$0$	$0$	$0$
optionalSelectInput_ui	💚	$6.93$	$-4.02$	$+2$	$0$	$0$	$-1$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
draggable_buckets	👶		$+2.46$	e2e_teal.widgets_draggable_buckets_initializes_with_default_inputs
draggable_buckets	👶		$+3.26$	e2e_teal.widgets_draggable_buckets_initializes_without_input
draggable_buckets	👶		$+2.95$	e2e_teal.widgets_draggable_buckets_moving_elements_between_buckets_updates_input
draggable_buckets_ui	💀	$3.25$	$-3.25$	e2e_teal.widgets_draggable_buckets_initializes
optionalSelectInput_ui	💚	$6.93$	$-4.02$	e2e_teal.widgets_optionalSelectInput_initializes

Results for commit 641d9b6

♻️ This comment has been updated with latest results.

[llrs-roche]

Looks good but I cannot really test on my computer for the moment (somehow still it doesn't show the output).

But I have 2 comments:

1. Will it work with old version of R/shiny? It is not clear from the linked PR on shiny if this is back compatible or not.
2. Could you add tests to make sure this works well in the future? Maybe raise this with the shiny maintainer that this broke compatibility with previous existing widgets...

testthat::test_that(
  "e2e: teal.widgets::draggable_buckets: initializes without input",
  {
    skip_if_too_deep(5)
    app_driver <- shinytest2::AppDriver$new(
      app_driver_db(),
      name = "db",
      variant = "app_driver_db_ui"
    )
    values <- app_driver$get_values()
    testthat::expect_true(is.null(unlist(values)))
    testthat::expect_length(values$output, 0L)
    app_driver$stop()
  }
)

testthat::test_that(
  "e2e: teal.widgets::draggable_buckets: moving element returns values",
  {
    skip_if_too_deep(5)
    app_driver <- shinytest2::AppDriver$new(
      app_driver_db(),
      name = "db",
      variant = "app_driver_db_ui"
    )
    # Move elements
    app_driver$run_js("
            const elementToMove = document.getElementById('id-draggable-1');
            const targetBucket = document.querySelector(`.bucket[data-label='bucket1']`);
            targetBucket.appendChild(elementToMove);
            Shiny.onInputChange('id', updatedValue);
                      ")

    values <- app_driver$get_values()
    testthat::expect_false(is.null(unlist(values)))
    app_driver$stop()
  }
)

They might need some tweaking as the second one failed locally but you get the idea.

[vedhav]

Yes, it is backward compatible. I tested with both shiny version 1.10.0 and 1.11.1. Perhaps we can ask someone else to test as you have some issue with your local setup.

[llrs-roche]

Restarting the computer fixed the issues with displaying the output. Please address add tests and I'll approve the PR.

[vedhav]

Hey @llrs-roche as I mentioned in the stand-up. There is no real way to test this with our current testing toolkit. I'm thinking of implementing this in teal.gallery. With cypress we get more testing capabilities.
And, we were calling the callback function incorrectly in our subscribe before. That's why this change in shiny broke it. They already made sure that it is backwards compatible.

[llrs-roche]

I don't know what cypress is, it doesn't seem to be an R package. Is this testing suit you refer to?

If we can't test the output once the element is moved, let's test at least that the elements are movable.
With these lines the first element moved to the first bucket.

const elementToMove = document.getElementById('iddraggable1'); 
const targetBucket = document.querySelector(`.bucket[data-label='bucket1']`);
targetBucket.appendChild(elementToMove);

To test the output we could create the widget with an element in one of the buckets. Per documentation, something like this should work:

app_driver_db <- function() {
  ui <- bslib::page_fluid(
    draggable_buckets("id", "Choices #1", c("a"), c(bucket1 = "b", bucket2 = NULL))
  )

  shiny::shinyApp(ui, function(input, output) {})
}

Then we can test that "b" is returned as output for bucket1.

[vedhav]

I've added the tests as we spoke about. Although they only test the second half of the widget functionality. We can continue the e2e tests in teal.gallery.

[llrs-roche]

Thanks for adding the tests. Approving, but please make the scope of the 4th test more limited as it fails on my local machine (Perhaps my locales affect the order).