init reporter tmc #518
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -514,6 +514,14 @@ ui_t_tte <- function(id, ...) { | |
| teal.widgets::standard_layout( | ||
| output = teal.widgets::white_small_well(teal.widgets::table_with_settings_ui(ns("table"))), | ||
| encoding = shiny::div( | ||
| ### Reporter | ||
| shiny::tags$div( | ||
| teal.reporter::add_card_button_ui(ns("addReportCard")), | ||
| teal.reporter::download_report_button_ui(ns("downloadButton")), | ||
| teal.reporter::reset_report_button_ui(ns("resetButton")) | ||
| ), | ||
| shiny::tags$br(), | ||
| ### | ||
| shiny::tags$label("Encodings", class = "text-primary"), | ||
| teal.transform::datanames_input( | ||
| a[c("arm_var", "paramcd", "aval_var", "cnsr_var", "strata_var", "event_desc_var")] | ||
|
|
@@ -681,6 +689,7 @@ ui_t_tte <- function(id, ...) { | |
| #' @noRd | ||
| srv_t_tte <- function(id, | ||
| datasets, | ||
| reporter, | ||
| arm_var, | ||
| paramcd, | ||
| aval_var, | ||
|
|
@@ -695,6 +704,7 @@ srv_t_tte <- function(id, | |
| label, | ||
| basic_table_args) { | ||
| stopifnot(is_cdisc_data(datasets)) | ||
| with_reporter <- !missing(reporter) && inherits(reporter, "Reporter") | ||
| shiny::moduleServer(id, function(input, output, session) { | ||
| teal.code::init_chunks() | ||
|
|
||
|
|
@@ -878,5 +888,35 @@ srv_t_tte <- function(id, | |
| ), | ||
| modal_title = label | ||
| ) | ||
|
|
||
| ### REPORTER | ||
| if (with_reporter) { | ||
| card_fun <- function(comment) { | ||
| card <- teal.reporter::TealReportCard$new() | ||
| card$set_name("Time To Event Table") | ||
| card$append_text("Time To Event Table", "header2") | ||
| card$append_text("Filter State", "header3") | ||
| card$append_fs(datasets) | ||
|
There was a problem hiding this comment. @gogonzo are there implication here using There was a problem hiding this comment. we need not only state, we are need the format method. That is why we need to pass here datasets as a whole. This was surprising for me and @mhallal1 to have this FilterData argument here , still we not oppose to it at that moment. There was a problem hiding this comment. yeah it's fine for now, I imagine once the state of the filter panel is decoupled from the data then this will need to be changed - but that's ok There was a problem hiding this comment. This will be affected once we decide to push data instead of datasets. This means that we will probably $add_text and $add_metadata directly. |
||
| card$append_text("Main Element", "header3") | ||
| card$append_table(table()) | ||
| if (!comment == "") { | ||
| card$append_text("Comment", "header3") | ||
| card$append_text(comment) | ||
| } | ||
| card$append_text("Show R Code", "header3") | ||
| card$append_src(paste(get_rcode( | ||
| chunks = teal.code::get_chunks_object(parent_idx = 1L), | ||
| datasets = datasets, | ||
| title = "", | ||
| description = "" | ||
| ), collapse = "\n")) | ||
| card | ||
| } | ||
|
|
||
| teal.reporter::add_card_button_srv("addReportCard", reporter = reporter, card_fun = card_fun) | ||
| teal.reporter::download_report_button_srv("downloadButton", reporter = reporter) | ||
|
There was a problem hiding this comment. I know that it's not for tmc but rather teal.reporter itself but let me comment that as this is the very first time we can see it in action. There was a problem hiding this comment. we assume to leave the Download/Reset buttons there from the beginning . This gives more flexible journey for an ad-hoc analysis, when sb does not have time to jump into the Previewer. Moreover we assume that more options is better than less. There was a problem hiding this comment. so if we want we can remove the download button here and in the UI for modules without needing to change teal.reporter I think There was a problem hiding this comment. yes, it is as easy as to remove 2 lines of code in the UI and server each (for each tmc module). Still I am not recommending it. |
||
| teal.reporter::reset_report_button_srv("resetButton", reporter) | ||
| } | ||
| ### | ||
| }) | ||
| } | ||
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.
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.
The only place I feel encoding could be valuable is MMRM, still in the first table visible after estimation we see most of the important parameters.
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.
True but you click the Add card button in the modal and nothing happens (i.e. there's no feedback to the user as to what is going on and they have to click cancel)
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'm not sure about that - for example in the KM plot in the example we have these fixed:
but they could be configurable? And their output is not on the plot. Similarly we are stratifying by SEX by default that is not included and can be changed
And also these are probably useful:
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 could investigate it tomorrow. For sure is not a part of this PR more a candidate for an issue in the teal.reporter.
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.
stratification, comparison settings would be nice to be added as footnotes as those are non-critical enough to be on the subtitles session
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.
The redundant click in the MMRM is solved here insightsengineering/teal.reporter#87
I added the
tryclause and print a proper message if the process failed.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.
Thanks all for work on the MMRM module, please check with me for any larger scale changes on functionality etc.
Also I am wondering if it would be better to split out the MMRM module mid-term (few months) in a separate package. Since then we could split this off the main NEST release and have it shipped separately with mmrm, tern.mmrm, teal.modules.mmrm outside of NEST - no longer limited by infrastructure problems etc.
@pawelru fyi re: our discussion today.
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.
Yeah it sounds good to split things up sensibly - my only concern would be as the number of packages we've created grows - we need a proper support model for maintaining/updating/releasing them all - even for trivial fixes - @dinakar29 @pawelru and I'm not clear we have one yet - e.g. internal users will still need these packages available internally (not on old infrastructure of course) and things will break and/or become inconsistent as newer versions of R/R packages are released.
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.
maintaining - better integration tests
updating/releasing - simpler release model
I have chatted a little with Dinakar and I might have an idea for the latter one. But this goes way beyond that PR so let's close this topic not to destroy the review.