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

Filter Panel - global turn on/off button #123

Merged
merged 38 commits into from
Nov 16, 2022
Merged

Filter Panel - global turn on/off button #123

merged 38 commits into from
Nov 16, 2022

Conversation

Polkas
Copy link
Contributor

@Polkas Polkas commented Nov 8, 2022

closes #84

Global toggle for Filter Panel, where the hidden state is recovered when turn on.
The current UI design is following recommended by Pawel and Nina direction.
The PR is taken into account the filter_panel_api which is linked with it.

Screenshot 2022-10-17 at 18 00 49

Please check out the discussion in #91

@Polkas Polkas added the core label Nov 8, 2022
Signed-off-by: Maciej Nasinski <[email protected]>
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

I like the idea and implementation. I also prefer to remove and restore filters than having two reactives for enable/disable.

R/FilteredData.R Outdated Show resolved Hide resolved
R/filter_panel_api.R Show resolved Hide resolved
Polkas and others added 2 commits November 10, 2022 12:01
Co-authored-by: Dony Unardi <[email protected]>
Signed-off-by: Maciej Nasinski <[email protected]>
Co-authored-by: Dony Unardi <[email protected]>
Signed-off-by: Maciej Nasinski <[email protected]>
R/filter_panel_api.R Outdated Show resolved Hide resolved
@Polkas Polkas marked this pull request as ready for review November 10, 2022 13:47
@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2022

Unit Tests Summary

    1 files    30 suites   20s ⏱️
442 tests 442 ✔️ 0 💤 0
776 runs  776 ✔️ 0 💤 0

Results for commit 5018a97.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2022

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/call_utils.R                  156       4  97.44%   142-145
R/CDISCFilteredData.R           152      28  81.58%   92, 115, 124, 128, 142-151, 255-262, 280-283, 289, 354-357
R/choices_labeled.R              49      14  71.43%   19, 30, 35, 45-50, 62, 66-70
R/eval_expr_with_msg.R           16       6  62.50%   8-13
R/filter_panel_api.R             23       8  65.22%   83, 111-117
R/filtered_data_wrappers.R       17      17  0.00%    134-169
R/FilteredData.R                469     248  47.12%   131, 228-232, 274-288, 329-343, 639-771, 799-800, 802, 833-835, 837-838, 846, 852-857, 875-880, 903-911, 914-939, 954-969, 1021-1042, 1066
R/FilteredDataset.R             263      65  75.29%   103, 199, 288, 352-392, 461-467, 504-514, 683-690
R/FilterState.R                1383     706  48.95%   160, 353-354, 468, 653-667, 729-779, 920-947, 1089-1192, 1217-1223, 1397-1557, 1703, 1713-1719, 1732, 1736, 1869-1979, 2036-2042, 2055, 2187-2306, 2334-2340, 2353, 2439-2441, 2506-2667, 2696-2702, 2716
R/FilterStates.R               1417     578  59.21%   104, 123, 227, 306, 385, 436-626, 640-641, 767-769, 773-781, 783, 787-789, 791, 950-959, 1016, 1082, 1122, 1183-1188, 1323-1444, 1459, 1492, 1560-1565, 1585-1591, 1598-1603, 1610-1611, 1613, 1774-1783, 1816-1823, 1831-1838, 1866-2035, 2079, 2098-2104, 2111-2116, 2124-2125, 2127, 2175-2176, 2257-2375, 2422, 2468, 2489
R/include_css_js.R                5       5  0.00%    12-16
R/init_filitered_data.R          62       0  100.00%
R/MAEFilteredDataset.R          215      59  72.56%   25, 126, 258-334
R/Queue.R                        50      23  54.00%   22, 40-72, 96-113, 154
R/resolve_state.R                23       0  100.00%
R/utils.R                        79      15  81.01%   115-121, 133-138, 211-212
R/variable_types.R               48      33  31.25%   42-47, 57, 70-105
R/zzz.R                           6       6  0.00%    3-11
TOTAL                          4433    1815  59.06%

Diff against main

Filename              Stmts      Miss  Cover
--------------------  -------  ------  -------
R/filter_panel_api.R  +13          +8  -34.78%
R/FilteredData.R      +44         -42  +15.36%
R/FilteredDataset.R   -           -19  +7.22%
R/FilterStates.R      -           -17  +1.20%
TOTAL                 +57         -70  +2.13%

Results for commit: be62bb3

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

R/filter_panel_api.R Outdated Show resolved Hide resolved
R/FilteredData.R Outdated
@@ -904,6 +972,12 @@ FilteredData <- R6::R6Class( # nolint
# private attributes ----
filtered_datasets = list(),

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

@pawelru pawelru Nov 14, 2022

Choose a reason for hiding this comment

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

(copied from closed PR as I still think it's relevant)


  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.

Originally posted by @pawelru in #91 (comment)


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.

Originally posted by @Polkas in #91 (comment)

Copy link
Contributor Author

@Polkas Polkas Nov 14, 2022

Choose a reason for hiding this comment

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

  1. I am ok with the active naming convention.
  2. I feel you need a little bit of background and support. I am happy to help you here.

Copy link
Contributor Author

@Polkas Polkas Nov 14, 2022

Choose a reason for hiding this comment

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

@pawelru you did not answer to more important question which is if we want to support filter panel api at all or in what scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

we had a quick meeting to discuss the above and posting the outcomes just for the record

  • the support/maintanance question is beyond this PR - a separate task will be has been created to discuss and decide
  • clarifications of my point: I meant this functionality will be mostly implemented on frontend side - not really in the backend (as shiny part is already inside this class definition it has to be a separate method but I hope you got the point). This would keep this class much simpler. This also means that one might call object methods on inactive filter that doesn't make much sense but I am totally fine with it.

R/filter_panel_api.R Outdated Show resolved Hide resolved
Copy link
Contributor

@mhallal1 mhallal1 left a comment

Choose a reason for hiding this comment

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

Can we enhance the toggle a bit to make it clear what it does?
Screenshot from 2022-11-14 12-12-00

@Polkas
Copy link
Contributor Author

Polkas commented Nov 14, 2022

@mhallal1 the help text will appear when you hover the slider. The minimalist design was applied on purpose.
Thank you for the support.

DESCRIPTION Outdated Show resolved Hide resolved
Signed-off-by: Maciej Nasinski <[email protected]>
@Polkas
Copy link
Contributor Author

Polkas commented Nov 14, 2022

linked to #127

R/FilteredData.R Outdated Show resolved Hide resolved
Signed-off-by: Maciej Nasinski <[email protected]>
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

👍 Tested apps and code - I'm happy to approve

@Polkas Polkas merged commit bdc6ef8 into main Nov 16, 2022
@Polkas Polkas deleted the 84_global_onoff@main branch November 16, 2022 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce disable filters in FilterPanelApi/FilteredData
6 participants