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

turn off on panel #91

Closed
wants to merge 19 commits into from
61 changes: 61 additions & 0 deletions R/FilteredData.R
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ FilteredData <- R6::R6Class( # nolint
)
}
logger::log_trace("FilteredData$set_filter_state initialized, dataname: { paste(names(state), collapse = ' ') }")

invisible(NULL)
},

Expand All @@ -516,6 +517,8 @@ FilteredData <- R6::R6Class( # nolint
#'
#' @return `NULL`
remove_filter_state = function(state) {
checkmate::assert_subset(names(state), self$datanames())

logger::log_trace("FilteredData$remove_filter_state called, dataname: { paste(names(state), collapse = ' ') }")

for (dataname in names(state)) {
Expand All @@ -524,6 +527,7 @@ FilteredData <- R6::R6Class( # nolint
}

logger::log_trace("FilteredData$remove_filter_state done, dataname: { paste(names(state), collapse = ' ') }")

invisible(NULL)
},

Expand Down Expand Up @@ -573,6 +577,33 @@ FilteredData <- R6::R6Class( # nolint
restore_state_from_bookmark = function(state, check_data_hash = TRUE) {
stop("Pure virtual method")
},
#' @description disable the filter panel
filter_panel_disable = function() {
private$filter_turn <- FALSE
shinyjs::disable("filter_add_vars")
shinyjs::disable("filter_active_vars")
private$cached_states <- self$get_filter_state()
self$remove_all_filter_states()
invisible(NULL)
},
#' @description enable the filter panel
filter_panel_enable = function() {
private$filter_turn <- TRUE
shinyjs::enable("filter_add_vars")
shinyjs::enable("filter_active_vars")
if (length(private$cached_states) && (length(self$get_filter_state()) == 0)) {
self$set_filter_state(private$cached_states)
}
invisible(NULL)
},
#' @description get the state of filter panel, if it is activated
get_filter_turn = function() {
private$filter_turn
},
#' @description get the id of the filter panel ui
get_filter_panel_ui_id = function() {
private$filter_panel_ui_id
},

# shiny modules -----

Expand All @@ -589,6 +620,17 @@ FilteredData <- R6::R6Class( # nolint
div(
id = ns(NULL), # used for hiding / showing
include_css_files(pattern = "filter-panel"),
div(
id = ns("switch-button"),
class = "inline-block",
shinyWidgets::switchInput(
ns("filter_turn_onoff"),
label = "TURN",
value = TRUE,
inline = FALSE,
handleWidth = "80px"
)
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please spend more time on UX here. IMO right now it is too maximal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the current UI as is obvious and the functionality important, please give me your idea then.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not about the importance of the feature - it's more how it fits into the overall GUI. Right now it doesn't fit well.
Maybe something like below? It's a minimal switch with decreased opacity.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the button that @Polkas proposed could work if we label it properly:

Screen Shot 2022-09-22 at 10 44 40 PM

But I also think the button that @pawelru proposed looks good too.

@kumamiao do you have any opinion on how which button that makes the most sense to the user?

Copy link
Contributor

@donyunardi donyunardi Sep 23, 2022

Choose a reason for hiding this comment

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

Also, @kumamiao, can I also get your opinion on the wording 'On/Off' or 'Enabled/Disabled' for the button toggle? Which one do you think user will prefer?

Choose a reason for hiding this comment

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

I am a fan of Pawel's proposition of the On/Off switch for Active filtering which is quite straightforward
image
The current button can be a bit misleading and takes more space

div(
id = ns("filters_overview"), # not used, can be used to customize CSS behavior
class = "well",
Expand Down Expand Up @@ -759,6 +801,18 @@ FilteredData <- R6::R6Class( # nolint
}
)

private$filter_panel_ui_id <- session$ns(NULL)
observeEvent(
eventExpr = input[["filter_turn_onoff"]],
handlerExpr = {
pawelru marked this conversation as resolved.
Show resolved Hide resolved
if (isTRUE(input[["filter_turn_onoff"]])) {
self$filter_panel_enable()
} else {
self$filter_panel_disable()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to put all of shinyjs::disable/enable calls here and use save/restore methods. This would keep shiny functionalities nicely decoupled - methods relates only to data fields and shiny parts lives inside the modules.
Moreover, I do not see (and I couldn't imagine) other place where we would call self$filter_panel_enable/disable().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"other place where we would call" - DRY rule is not the only reason to separate the code. Nice that you are raising it, please check how overcrowded is the server of filter panel now. WE should work to make it more clear and readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

DRY rule is not the only reason to separate the code

Yes you are definitely right here but my first point remains.
Long time ago we have a lengthy discussions with @gogonzo and Konrad about decoupling shiny-related parts from the class definitions. Unfortunately there is no good design pattern and we decided to still keep it inside the class definition but only inside one module method or two ui/server methods. This indeed means that those the method(s) definitions would be lengthy but it also assures nice decoupling of class' methods from the shiny part which should serve just as an add-on which calls those methods. Separate shiny-methods would break that and make the code less separated.

}, ignoreNULL = TRUE
)

observeEvent(
eventExpr = active_datanames(),
handlerExpr = {
Expand Down Expand Up @@ -891,6 +945,12 @@ FilteredData <- R6::R6Class( # nolint
# private attributes ----
filtered_datasets = list(),

# turn on / off filter panel
filter_turn = TRUE,
Copy link
Contributor

@pawelru pawelru Sep 2, 2022

Choose a reason for hiding this comment

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

  1. I dislike the "turn" term here. You can turn left/right or on/off. It's a verb whereas you are looking for a noun. I propose state / status / active instead. Please change other places as well including method docs.

  2. What's the point of this field? I ask from the class simplicity point of view.
    I can see that it's used in the api class - there is a check whether it's active and if yes then you can do set/get/etc. Is that needed? The functionality would be anyway disabled from the GUI perspective (vide: disabling inputs). What I am suggesting here is that the big disable/enable button would just propagate this action to all relevant inputs + save/restore action and that's it. An object would not store its state as this opens a door for unnecessary complexity I believe. I am happy to discuss it further if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default shinyWidgets::switchInput use the logical(1) for value so this should not be surprising I stay with this type.

filter_turn variable is needed only for filter_panel_api, so we could provide a proper warning if sb want to update the state when filter panel is turn off.

I like the Turn Off/On naming, although I see you agree with Nik in this area so please update to the one you are preferring.


# filter panel ui id
filter_panel_ui_id = character(0),

# whether the datasets had a reproducibility check
.check = FALSE,

Expand All @@ -902,6 +962,7 @@ FilteredData <- R6::R6Class( # nolint

# reactive i.e. filtered data
reactive_data = list(),
cached_states = NULL,

# we implement these functions as checks rather than returning logicals so they can
# give informative error messages immediately
Expand Down
34 changes: 30 additions & 4 deletions R/filter_panel_api.R
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ FilterPanelAPI <- R6::R6Class( # nolint
#'
#' @return `NULL`
set_filter_state = function(filter) {
private$filtered_data$set_filter_state(filter)
if (private$filtered_data$get_filter_turn()) {
private$filtered_data$set_filter_state(filter)
} else {
warning("Filter Panel is turn off so the action can not be applied with api.")
}
invisible(NULL)
},

Expand All @@ -73,7 +77,11 @@ FilterPanelAPI <- R6::R6Class( # nolint
#'
#' @return `NULL`
remove_filter_state = function(filter) {
private$filtered_data$remove_filter_state(filter)
if (private$filtered_data$get_filter_turn()) {
private$filtered_data$remove_filter_state(filter)
} else {
warning("Filter Panel is turn off so the action can not be applied with api.")
}
invisible(NULL)
},

Expand All @@ -83,8 +91,26 @@ FilterPanelAPI <- R6::R6Class( # nolint
#'
#' @return `NULL`
remove_all_filter_states = function(datanames) {
datanames_to_remove <- if (missing(datanames)) private$filtered_data$datanames() else datanames
private$filtered_data$remove_all_filter_states(datanames = datanames_to_remove)
if (private$filtered_data$get_filter_turn()) {
datanames_to_remove <- if (missing(datanames)) private$filtered_data$datanames() else datanames
private$filtered_data$remove_all_filter_states(datanames = datanames_to_remove)
} else {
warning("Filter Panel is turn off so the action can not be applied with api.")
}
invisible(NULL)
},
#' @description Toggle the state of the Filter Panel turn on/off button.
#' @param id (`character(1)`)\cr
#' `id` to the toggle button.
#'
#' @return `NULL`
filter_panel_toggle = function() {
shinyjs::runjs(
sprintf(
'$("#%s-filter_turn_onoff").click();',
private$filtered_data$get_filter_panel_ui_id()
)
)
invisible(NULL)
}
),
Expand Down
5 changes: 5 additions & 0 deletions inst/css/filter-panel.css
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@
position: relative;
}

.inline-block {
position: inline-block;
}

.float-right {
float: right;
}

.float-left {
float: left;
}
Expand Down
4 changes: 4 additions & 0 deletions man/CDISCFilteredData.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions man/FilterPanelAPI.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

52 changes: 48 additions & 4 deletions man/FilteredData.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 64 additions & 0 deletions tests/testthat/test-FilteredData.R
Original file line number Diff line number Diff line change
Expand Up @@ -576,3 +576,67 @@ testthat::test_that("get_data assert the `filtered` argument is logical(1)", {
regexp = "Assertion on 'filtered' failed: Must be of type 'logical flag', not 'character'"
)
})

testthat::test_that("filter_panel_disable", {
filtered_data <- FilteredData$new(data_objects = list("iris" = list(dataset = iris)), join_keys = NULL)
filtered_data$set_filter_state(list(iris = list(Sepal.Width = c(3, 4))))
shiny::testServer(
filtered_data$srv_filter_panel,
expr = {
filtered_data$filter_panel_disable()
testthat::expect_length(filtered_data$get_filter_state(), 0)
}
)
})

testthat::test_that("filter_panel_enable", {
filtered_data <- FilteredData$new(data_objects = list("iris" = list(dataset = iris)), join_keys = NULL)
filtered_data$set_filter_state(list(iris = list(Sepal.Width = c(3, 4))))
shiny::testServer(
filtered_data$srv_filter_panel,
expr = {
filtered_data$filter_panel_enable()
testthat::expect_length(filtered_data$get_filter_state(), 1)
testthat::expect_equal(filtered_data$get_filter_state()$iris$Sepal.Width$selected, c(3, 4))
}
)
})

testthat::test_that("filter_panel_disable and filter_panel_enable", {
filtered_data <- FilteredData$new(data_objects = list("iris" = list(dataset = iris)), join_keys = NULL)
filtered_data$set_filter_state(list(iris = list(Sepal.Width = c(3, 4))))
shiny::testServer(
filtered_data$srv_filter_panel,
expr = {
testthat::expect_length(filtered_data$get_filter_state(), 1)
testthat::expect_true(filtered_data$get_filter_turn())
filtered_data$filter_panel_disable()
testthat::expect_length(filtered_data$get_filter_state(), 0)
testthat::expect_false(filtered_data$get_filter_turn())
filtered_data$filter_panel_enable()
testthat::expect_length(filtered_data$get_filter_state(), 1)
testthat::expect_true(filtered_data$get_filter_turn())
}
)
})

testthat::test_that("turn filed by default equal to TRUE", {
filtered_data <- FilteredData$new(data_objects = list("iris" = list(dataset = iris)), join_keys = NULL)
testthat::expect_true(filtered_data$get_filter_turn())
})

testthat::test_that("get_filter_panel_ui_id - empty when no shiny session", {
filtered_data <- FilteredData$new(data_objects = list("iris" = list(dataset = iris)), join_keys = NULL)
testthat::expect_length(filtered_data$get_filter_panel_ui_id(), 0)
})

testthat::test_that("get_filter_panel_ui_id - non-empty when in shiny session", {
filtered_data <- FilteredData$new(data_objects = list("iris" = list(dataset = iris)), join_keys = NULL)
shiny::testServer(
filtered_data$srv_filter_panel,
expr = {
testthat::expect_length(filtered_data$get_filter_panel_ui_id(), 1)
}
)
})

Loading