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: duplicated slices are lingering the filter panel after DDL reload #602

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

averissimo
Copy link
Contributor

@averissimo averissimo commented Jul 26, 2024

Pull Request

Companion of insightsengineering/teal#1275

Note: There's a similar solution that is cleaner. I'll push it on Monday

Changes description

  • Keeps track of remove observers on filter panel
  • Destroys remove observer when clearing filter panel

@averissimo averissimo requested a review from gogonzo July 26, 2024 13:54
@gogonzo gogonzo self-assigned this Jul 29, 2024
R/FilterStates.R Outdated

# Each filter state_id can only have 1 active remove observer at the time
remove_id <- sprintf("remove_%s", state_id)
if (!is.null(private$observers[[remove_id]])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here. If we have one instance of FilteredData and multiple srv_active (for each module), then it is removing observers from other tabs. For example when module_specific = FALSE and two modules use the same filter - it will be possible to remove filter from the last-rendered tab only.

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 still need to keep track of the observers that are created.

I removed the overwrite, but keeping any additional observers with remove_<state_id>_<count>

Any observer that has the state id on its name is removed on method call state_list_remove via

state$destroy_observers() # already
lapply(
  Filter(function(x) grepl(state$get_state()$id, x, fixed = TRUE), names(private$observers)),
  function(x) private$observers[[x]]$destroy()
)

Ideally we would push the observer to the state$private$observers, but that is private and requires new API on <teal.slice>/R/FilterState.R

R/FilterStates.R Outdated
Comment on lines 385 to 387
remove_id_prefix <- sprintf("remove_%s", state_id)
remove_id <- sprintf("%s_%s", remove_id_prefix, sum(grepl(remove_id_prefix, names(private$observers))))
private$observers[[remove_id]] <- remove_observer
Copy link
Contributor

Choose a reason for hiding this comment

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

For sake of simplicity, you can also:

  • move FilterState$<private>$observers to FilterState$<public>$observers
  • set this remove_observer as an element of FilterState$observers class
  • then state_list_remove can remain unchanged and will call state$destroy_observers()

@gogonzo gogonzo merged commit 0c90543 into 669_insertUI@main Aug 1, 2024
1 check passed
@gogonzo gogonzo deleted the teal_slice@669_insertUI@main branch August 1, 2024 03:37
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2024
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