-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Changes from 9 commits
ffa764e
ee8188e
ef37b85
d1fbd69
59d9f5e
0c16055
7735031
f4dc8ca
71729a7
b4a8483
1eb1ffe
e49e2a1
cf0367a
56e3a4c
91b1ce3
fd185ea
13345c6
d57ca8f
728454f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -506,6 +506,7 @@ FilteredData <- R6::R6Class( # nolint | |
) | ||
} | ||
logger::log_trace("FilteredData$set_filter_state initialized, dataname: { paste(names(state), collapse = ' ') }") | ||
|
||
invisible(NULL) | ||
}, | ||
|
||
|
@@ -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)) { | ||
|
@@ -524,6 +527,7 @@ FilteredData <- R6::R6Class( # nolint | |
} | ||
|
||
logger::log_trace("FilteredData$remove_filter_state done, dataname: { paste(names(state), collapse = ' ') }") | ||
|
||
invisible(NULL) | ||
}, | ||
|
||
|
@@ -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 ----- | ||
|
||
|
@@ -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" | ||
) | ||
), | ||
div( | ||
id = ns("filters_overview"), # not used, can be used to customize CSS behavior | ||
class = "well", | ||
|
@@ -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() | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest to put all of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes you are definitely right here but my first point remains. |
||
}, ignoreNULL = TRUE | ||
) | ||
|
||
observeEvent( | ||
eventExpr = active_datanames(), | ||
handlerExpr = { | ||
|
@@ -891,6 +945,12 @@ FilteredData <- R6::R6Class( # nolint | |
# private attributes ---- | ||
filtered_datasets = list(), | ||
|
||
# turn on / off filter panel | ||
filter_turn = TRUE, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By default
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, | ||
|
||
|
@@ -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 | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
![image](https://user-images.githubusercontent.com/12943682/190375631-d86e62c5-5fa8-4290-adba-32cfce565b13.png)
Maybe something like below? It's a minimal switch with decreased opacity.
There was a problem hiding this comment.
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:
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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](https://user-images.githubusercontent.com/22608476/192032035-e34ac8fd-49ce-4515-b7ea-84b12b97eb97.png)
The current button can be a bit misleading and takes more space