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 +/- add new filter buttons when changing data using teal_data_module #1282

Closed

Conversation

vedhav
Copy link
Contributor

@vedhav vedhav commented Jul 31, 2024

Fixes the +/- icon behavior on the filter panel.
Please checkout to the fix-add-remove-ui@669_insertUI@main in both teal and teal.slice
In teal.slice it just reverts one commit to the working version of the show/hide logic.

When the data changes using teal_data_module the filter panel observers are being called once again thus running the scope of the observers as many times the data changes.
The fix is to reinitialize the filter panel with new NS so the observers are not connected to the active UI anymore.

Example app to test

pkgload::load_all("teal.slice")
pkgload::load_all("teal")
library(MultiAssayExperiment)

data <- teal::teal_data_module(
  ui = function(id) {
    ns <- shiny::NS(id)
    shiny::tagList(
      shiny::textInput(ns("username"), label = "Username"),
      shiny::passwordInput(ns("password"), label = "Password"),
      shiny::actionButton(ns("submit"), label = "Submit")
    )
  },
  server = function(id, ...) {
    shiny::moduleServer(id, function(input, output, session) {
      shiny::eventReactive(input$submit, {
        data <- teal.data::teal_data() |>
          within(
            {
              logger::log_debug("Loading data...")
              iris <- iris
              data("miniACC", package = "MultiAssayExperiment", envir = environment())
            },
            username = input$username,
            password = input$password
          )
        data
      })
    })
  },
  once = FALSE
)

app <- init(
  data = data,
  modules = example_module()
)

shiny::runApp(app$ui, app$server)

@vedhav vedhav added the core label Jul 31, 2024
@vedhav vedhav requested a review from gogonzo July 31, 2024 05:12
@gogonzo
Copy link
Contributor

gogonzo commented Jul 31, 2024

This will complicate testing a little bit. What are the alternatives?
Why not to make a PR to teal.slice so we can discuss those changes?

@vedhav
Copy link
Contributor Author

vedhav commented Jul 31, 2024

Raised the PR in teal.slice. (It is just reverting one commit. So, changes over there are not relevant to the fix.)
But, this solves a wider issue of not having to call multiple observers, without this all the observers in the srv_active are being called as many times as the filter module is initialized.
The alternative would be to implement the observer destroying logic we use in FilterState by storing the observers and destroying them. However, we might have to make that observer public as we need to trigger the destroy in teal when we call this module.

@vedhav
Copy link
Contributor Author

vedhav commented Aug 5, 2024

Closing this as the multiple observers issue is fixed by destroying the observers in teal.slice.

@insights-engineering-bot insights-engineering-bot deleted the fix-add-remove-ui@669_insertUI@main branch November 3, 2024 03:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants