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

187 new api@filter panel refactor@main #825

Merged

Conversation

chlebowa
Copy link
Contributor

Introduces changes necessary to handle the new Filter Panel API coming from this PR.

gogonzo and others added 5 commits March 17, 2023 08:37
Fixes a few documentation issues that need to be addressed for release.
Note that not all items from #815 are in this PR. Some of them need
discussion, so I opened #818.

Closes #815
Closes [this
issue](insightsengineering/teal.modules.clinical#723).

Changed default values of `header` and `footer` in `init` from
`tags$p("Add title here")` to `tags$p()`.
@chlebowa chlebowa added the core label Apr 13, 2023
@chlebowa chlebowa marked this pull request as ready for review April 13, 2023 16:07
@chlebowa chlebowa requested a review from asbates April 13, 2023 16:07
@github-actions
Copy link
Contributor

Unit Tests Summary

    1 files    12 suites   10s ⏱️
143 tests 137 ✔️ 0 💤 0  6 🔥
279 runs  273 ✔️ 0 💤 0  6 🔥

For more details on these errors, see this check.

Results for commit 7f10a50.

@github-actions
Copy link
Contributor

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -----------------------------------
R/dummy_functions.R                 74      61  17.57%   12-95
R/example_module.R                  18       9  50.00%   23-26, 29-33
R/get_rcode_utils.R                 45       0  100.00%
R/include_css_js.R                  24       0  100.00%
R/init.R                            28       2  92.86%   202-203
R/module_nested_tabs.R             161       8  95.03%   57, 97, 101-102, 137, 188, 240, 266
R/module_tabs_with_filters.R        59       2  96.61%   162, 175
R/module_teal_with_splash.R         33       2  93.94%   65, 77
R/module_teal.R                    111       6  94.59%   49, 52, 155-156, 180, 218
R/modules_debugging.R               18      18  0.00%    37-56
R/modules.R                        101       8  92.08%   341-366
R/reporter_previewer_module.R       12       2  83.33%   18, 22
R/show_rcode_modal.R                20      20  0.00%    17-38
R/tdata.R                           41       2  95.12%   146, 172
R/utils.R                           13       0  100.00%
R/validate_inputs.R                 32       0  100.00%
R/validations.R                     62      39  37.10%   107-368
R/zzz.R                             11       7  36.36%   3-14
TOTAL                              863     186  78.45%

Diff against main

Filename                        Stmts    Miss  Cover
----------------------------  -------  ------  -------
R/init.R                           +4       0  +1.19%
R/module_nested_tabs.R              0      +1  -0.62%
R/module_tabs_with_filters.R       -9      +1  -1.92%
R/module_teal.R                     0      +1  -0.90%
TOTAL                              -5      +3  -0.47%

Results for commit: 845f1a5

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@asbates
Copy link
Contributor

asbates commented Apr 13, 2023

I get an error when running the cdsic example app in insightsengineering/teal.slice/pull/165. I am on branch 187_... for both teal and teal.slice

image

@donyunardi donyunardi changed the base branch from main to filter_panel_refactor@main April 13, 2023 20:53
@donyunardi
Copy link
Contributor

I think the example code fails because the default_filters object is not using the new API structure with filter_settings and filter_vars.

So, when I changed the default_filters to this:

default_filters <- filter_settings(
    filter_var(dataname = "ADSL", varname = "categorical2", selected = c("a", "b")),
    filter_var(dataname = "ADSL", varname = "numeric", selected = c(0, 140), keep_na = TRUE, keep_inf = TRUE),
    filter_var(dataname = "ADTTE", varname = "AVAL", selected = c(10, 1000))
)

It works.

However, I ran into couple issues:

1. Got a new error message, but the app still work:

This is what I see in the console:

Listening on http://127.0.0.1:6104
[WARN] 2023-04-13 14:18:16.3260 pid:72894 token:[cafcf5b4] teal.slice Programmatic range specification on numeric was adjusted to existing slider ticks. It is now broader in order to contain the specified values.
[WARN] 2023-04-13 14:18:16.3598 pid:72894 token:[cafcf5b4] teal.slice Programmatic range specification on AVAL was adjusted to existing slider ticks. It is now broader in order to contain the specified values.
Warning: Error in ggplot2::geom_histogram: Problem while computing aesthetics.Error occurred in the 1st layer.
Caused by error in `check_aesthetics()`:
! Aesthetics must be either length 1 or the same as the data (381)
✖ Fix the following mappings: `x`
  238: <Anonymous>
  237: signalCondition
  236: signal_abort
  235: rlang::abort
  234: cli::cli_abort
  233: handlers[[1L]]
  232: <Anonymous>
  231: signalCondition
  230: signal_abort
  229: rlang::abort
  228: cli::cli_abort
  227: check_aesthetics
  226: compute_aesthetics
  225: l$compute_aesthetics
  224: f
  217: by_layer
  216: ggplot_build.ggplot
  214: print.ggplot
  209: func
  207: f
  206: Reduce
  197: do
  196: hybrid_chain
  168: drawPlot
  154: <reactive:plotObj>
  138: drawReactive
  125: renderFunc
  124: valueFunc
  111: valueFunc
   96: func
   94: f
   93: Reduce
   84: do
   83: hybrid_chain
   82: output$teal-main_ui-filter_panel-active-ADSL-filter-_var_numeric-card-inputs-plot
    1: runApp
Input to asJSON(keep_vec_names=TRUE) is a named vector. In a future version of jsonlite, this option will not be supported, and named vectors will be translated into arrays instead of objects. If you want JSON object output, please use a named list instead. See ?toJSON.

2. I can't seem to get the default filter working for column with logical value.

If I add the logical field to the default_filters with the setup below, the app crashed:

default_filters <- filter_settings(
    filter_var(dataname = "ADSL", varname = "logical"),
    filter_var(dataname = "ADSL", varname = "logical1", selected = FALSE)
)

Maybe we should make a new issue on this but want to see if someone else can reproduce my error.

@donyunardi
Copy link
Contributor

I guess another question is, do we need to retain the old way to setup filter state while we introduce the new API?

@asbates
Copy link
Contributor

asbates commented Apr 13, 2023

@donyunardi I definitely should have updated it to the new API. Can't believe I forgot!

Whether we need to support the old way, I think that support was added. Is that right @chlebowa ?

I am getting the same error as well.

@chlebowa
Copy link
Contributor Author

chlebowa commented Apr 14, 2023

  1. Filter specification should work with both the old and the new way and it does for me. If it fails on your machine, I suggest a face to face to figure it out.
  2. The second error, with the logical column, was caused by the format method and is now fixed.
  3. I will look into the slider thing.

Update: the slider error only seems to appear in the CDISC app, not in the non-CDISC and MAE ones.

@chlebowa
Copy link
Contributor Author

slider bug is fixed

@chlebowa chlebowa merged commit beb3b0a into filter_panel_refactor@main Apr 14, 2023
@chlebowa chlebowa deleted the 187_new_api@filter_panel_refactor@main branch April 14, 2023 14:34
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.

None yet

4 participants