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

Fix condition for calling exports.unbindAll() #1736

Merged
merged 2 commits into from
Jun 15, 2017
Merged

Fix condition for calling exports.unbindAll() #1736

merged 2 commits into from
Jun 15, 2017

Conversation

wch
Copy link
Collaborator

@wch wch commented Jun 8, 2017

This is a correction to #1449. That change had no effect on behavior.

With the example app from rstudio/DT#369, if you select rows in the first datatable, and then add another table by moving the slider, the selected rows get unselected, with the current master.

library(shiny)
library(DT)

numUI <- 0

ui <- shinyUI(fluidPage(
  mainPanel(
    sliderInput("number","Number of tables",1,10,1,1),
    tags$div(id="tables")
  )))

server <- shinyServer(function(input, output, session) {

  observe({
    if (input$number > numUI) {
      for (num in (numUI+1):input$number) {
        insertUI("#tables", "beforeEnd", DT::dataTableOutput(paste0("table", num)))
        output[[paste0("table",num)]] <- DT::renderDataTable(head(mtcars), server = FALSE)
      }
    }

    if (input$number < numUI) {
      for (num in (input$number+1):numUI) {
        removeUI(paste0("#table", num))
      }
    }

    numUI <<- input$number
  })

})
# Run the application
shinyApp(ui = ui, server = server)

After this change, the rows do not get unselected when adding a table.

@wch wch added the review label Jun 8, 2017
@wch
Copy link
Collaborator Author

wch commented Jun 9, 2017

This also should have some tests with shinytest, possibly in a new repo added with a git submodule or subtree.

@bborgesr
Copy link
Contributor

Here's a simpler test app:

ui <- fluidPage(
  actionButton("add", "Add slider"),
  tags$div(id = "sliders")
)

server <- function(input, output, session) {
  observeEvent(input$add, {
    containerID <- paste0("ui-", input$add)
    sliderID <- paste0("slider-", input$add)
    insertUI("#sliders", "beforeEnd", uiOutput(containerID))
    output[[containerID]] <- renderUI(sliderInput(sliderID, sliderID, 0, 10, 5))
  }, ignoreInit = TRUE)
}

shinyApp(ui, server)

I have started a test repo (rstudio/shinyTestResults), but neither this app nor the first one (using DT) can be tested properly using shinytest (see rstudio/shinytest#105 and rstudio/shinytest#106). So, I think we should merge for now while Winston works on those shinytest edge cases (since the problem is with shinytest, not with this PR). It is clear that the PR works if we test manually.

@wch wch merged commit 2158f90 into master Jun 15, 2017
@wch wch deleted the wch-fix-unbind branch June 15, 2017 18:05
@wch wch removed the review label Jun 15, 2017
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

2 participants