turn off on panel #91

Polkas · 2022-08-22T15:36:39Z

closes #84

Global toggle for Filter Panel, where the hidden state is recovered when turn on.
The PR is taken into account the filter_panel_api which is linked with it.

nikolas-burkoff · 2022-08-23T09:03:53Z

Does this close #84 - I guess you also need the trigger in FilteredDataAPI

R/FilteredData.R

nikolas-burkoff · 2022-08-23T09:10:00Z

We also need to correctly (whatever that means) handle the case where the filter panel is disabled but the module developer calls the filter panel api to set states

Polkas · 2022-08-23T09:49:27Z

Does this close #84 - I guess you also need the trigger in FilteredDataAPI

No, this is a button in the global scope not local.

nikolas-burkoff · 2022-08-26T10:43:15Z

Do you think there's a way of linking all these together (i.e. some panel with light shading to show that the whole of this is the filter panel?) - and maybe a title "filtering"

Also I don't like the word "Turn" there maybe An inline label "Filtering:" then the switch you have with labels "Enabled" + "Disabled"
There's a picture of the filter panel in the teal vignette which I think should be updated (or a ticket added to update it)

I'm aware there is a big filter panel UI change coming so not sure whether these should be done here or not

gogonzo

tests are missing, besides looks good to me

pawelru · 2022-09-02T10:12:35Z

R/FilteredData.R

@@ -891,6 +945,12 @@ FilteredData <- R6::R6Class( # nolint
    # private attributes ----
    filtered_datasets = list(),

+    # turn on / off filter panel
+    filter_turn = TRUE,


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.

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.

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.

pawelru · 2022-09-02T10:22:03Z

R/FilteredData.R

+            handlerExpr = {
+              if (isTRUE(input[["filter_turn_onoff"]])) {
+                self$filter_panel_enable()
+              } else {
+                self$filter_panel_disable()
+              }


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().

"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.

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.

R/FilteredData.R

pawelru · 2022-09-02T10:28:08Z

R/FilteredData.R

+        div(
+          id = ns("switch-button"),
+          class = "inline-block",
+          shinyWidgets::switchInput(
+            ns("filter_turn_onoff"),
+            label = "TURN",
+            value = TRUE,
+            inline = FALSE,
+            handleWidth = "80px"
+          )
+        ),


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

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

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.

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?

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?

I am a fan of Pawel's proposition of the On/Off switch for Active filtering which is quite straightforward

The current button can be a bit misleading and takes more space

Polkas · 2022-10-17T12:13:46Z

Finally the idea from @kumamiao and @pawelru looks most preferable so I apply the shinyWidgets::prettySwitch. The implementation is very straightforward and equally easy to maintain.

pawelru · 2022-10-17T13:27:20Z

Thank you - this is much better now!

The screenshot you provided made me think about yet another idea - how about placing the switch next to the show/hide button (hamburger one). Those are conceptually functionalities affecting the same object - filter. WDYT?

Polkas · 2022-10-17T16:05:18Z

@pawelru very interesting proposition.
So the hamburger button is a part of teal::init not the filtering panel by itself.
Currently we are adding the activate button directly to the filter panel, so under current design I could not bind them together directly. I could make the switch button to align right by itself.

I apply your proposition and I remove the text and leave it only as a title which will be printed on hover. I very like this scenario.

with regular text:

turn off on panel

ffa764e

nikolas-burkoff reviewed Aug 23, 2022

View reviewed changes

R/FilteredData.R Outdated Show resolved Hide resolved

disable

ee8188e

Polkas added 2 commits August 24, 2022 17:35

polish

ef37b85

docs

d1fbd69

Polkas added the core label Aug 25, 2022

nikolas-burkoff mentioned this pull request Aug 25, 2022

Introduce disable filters in FilterPanelApi/FilteredData #84

Closed

Polkas mentioned this pull request Aug 26, 2022

Apply Quosure and data in tmg insightsengineering/teal.modules.general#440

Merged

15 tasks

Polkas requested a review from gogonzo August 29, 2022 07:20

gogonzo reviewed Aug 29, 2022

View reviewed changes

Polkas added 5 commits August 29, 2022 15:52

tests

59d9f5e

tests

0c16055

tests

7735031

tests

f4dc8ca

tests

71729a7

pawelru reviewed Sep 2, 2022

View reviewed changes

R/FilteredData.R Show resolved Hide resolved

pawelru reviewed Sep 2, 2022

View reviewed changes

insightsengineering deleted a comment from pawelru Sep 2, 2022

Polkas added 2 commits September 2, 2022 12:44

logger

b4a8483

Merge branch 'main' into 26_new_chunks@main

1eb1ffe

nikolas-burkoff mentioned this pull request Sep 16, 2022

Merge the refactor branch insightsengineering/teal#731

Closed

8 tasks

donyunardi mentioned this pull request Oct 7, 2022

Simplify chunks (2) - wrap up qenv insightsengineering/NEST-roadmap#69

Closed

21 tasks

Nikolas Burkoff and others added 2 commits October 13, 2022 10:43

Merge branch 'main' into 26_new_chunks@main

e49e2a1

Merge branch 'main' into 26_new_chunks@main

cf0367a

shinyWidgets::prettySwitch

56e3a4c

Polkas added 2 commits October 17, 2022 18:03

right align

91b1ce3

styler

fd185ea

Polkas added 3 commits October 17, 2022 18:20

rm style

13345c6

Merge branch 'main' into 26_new_chunks@main

d57ca8f

Merge branch 'main' into 26_new_chunks@main

728454f

Polkas closed this Nov 8, 2022

Polkas mentioned this pull request Nov 8, 2022

Filter Panel - global turn on/off button #123

Merged

gogonzo deleted the 26_new_chunks@main branch January 13, 2023 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

turn off on panel #91

turn off on panel #91

Polkas commented Aug 22, 2022 •

edited

Loading

nikolas-burkoff commented Aug 23, 2022 •

edited

Loading

nikolas-burkoff commented Aug 23, 2022

Polkas commented Aug 23, 2022

nikolas-burkoff commented Aug 26, 2022

gogonzo left a comment

pawelru Sep 2, 2022 •

edited

Loading

Polkas Sep 2, 2022

pawelru Sep 2, 2022

Polkas Sep 2, 2022

pawelru Sep 2, 2022

pawelru Sep 2, 2022

Polkas Sep 2, 2022

pawelru Sep 15, 2022

donyunardi Sep 23, 2022

donyunardi Sep 23, 2022 •

edited

Loading

kumamiao Sep 23, 2022

Polkas commented Oct 17, 2022

pawelru commented Oct 17, 2022

Polkas commented Oct 17, 2022

turn off on panel #91

turn off on panel #91

Conversation

Polkas commented Aug 22, 2022 • edited Loading

nikolas-burkoff commented Aug 23, 2022 • edited Loading

nikolas-burkoff commented Aug 23, 2022

Polkas commented Aug 23, 2022

nikolas-burkoff commented Aug 26, 2022

gogonzo left a comment

Choose a reason for hiding this comment

pawelru Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donyunardi Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Polkas commented Oct 17, 2022

pawelru commented Oct 17, 2022

Polkas commented Oct 17, 2022

Polkas commented Aug 22, 2022 •

edited

Loading

nikolas-burkoff commented Aug 23, 2022 •

edited

Loading

pawelru Sep 2, 2022 •

edited

Loading

donyunardi Sep 23, 2022 •

edited

Loading