198 Include user's card labels when generating the report #336
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 068f65a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Thanks @kartikeyakirar that is nice! is there something that should go into the shiny tests accordingly?
@danielinteractive I haven't made any specific changes for shinytest, but I've included a test for |
There was a problem hiding this comment.
Thanks @kartikeyakirar , nice, please see last comments below, afterwards good to merge!
R/utils.R
Outdated
| #' @param filter_panel_api (`FilterPanelAPI`) object with API that allows the generation | ||
| #' of the filter state in the report | ||
| #' | ||
| #' @return (`TealReportCard`) populated with a title, description and filter state | ||
| #' | ||
| #' @keywords internal | ||
| card_template <- function(title, label, description = NULL, with_filter, filter_panel_api) { | ||
| card_template <- function(title, label, description = NULL, filter_panel_api) { | ||
| checkmate::assert_string(title) |
There was a problem hiding this comment.
sorry can we please take out the checkmate:: name space thing since we import the whole of it?
There was a problem hiding this comment.
putting package name in front of package function improves the process of debugging if you debug the function interactively
There was a problem hiding this comment.
teal is also imported as a whole but we do put :: in front of teal function names https://github.com/search?q=repo%3Ainsightsengineering%2Fteal.modules.hermes%20%3A%3A&type=code
teal.modules.hermes/R/scatterplot.R
Line 48 in 3d16439
There was a problem hiding this comment.
yeah we should also take out the teal:: bits please. thanks!
There was a problem hiding this comment.
@kartikeyakirar have you seen this conversation about teal
There was a problem hiding this comment.
Okay, I will take out teal:: call as well.
Edit : Namespace calls are throughout the package, and it's not within the scope of this task to eliminate all of them. However, I will remove teal::report_card_template. As for the rest, I will put the task to handle them separately .
There was a problem hiding this comment.
Here is task : #337
|
@kartikeyakirar please take out card_template and move it to teal.reporter. Move the test as well |
There was a problem hiding this comment.
Hey @kartikeyakirar looks like this staged deps cyclic error does not appear on this PR anymore. You are good to merge
#933) this PR is extension to insightsengineering/teal.reporter#198 Here I have added card_template() function that generates a report card with a title, an optional description, and the option to append the filter state list. and is been used in insightsengineering/teal.modules.general#584 insightsengineering/teal.modules.clinical#835 insightsengineering/teal.modules.hermes#336 insightsengineering/teal.osprey#229 insightsengineering/teal.goshawk#241 ref issues and discussion: insightsengineering/teal.reporter#226 --------- Signed-off-by: kartikeya kirar <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Marcin <[email protected]> Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
this PR fixes #335
this is follow-up after insightsengineering/teal.reporter#219