insightsengineering / teal.reporter

Create and preview reports with Shiny modules
https://insightsengineering.github.io/teal.reporter/
Other
8 stars 9 forks source link

198 Include user's card labels when generating the report #219

Closed m7pr closed 1 year ago

m7pr commented 1 year ago

Closes #198 and accompanied by https://github.com/insightsengineering/teal.modules.clinical/pull/835

The problem

Currently most of the teal modules, that wrap up teal.reporter in their UI, have titles of the ReportCard hardcoded in their body. For example check out below 2 modules

https://github.com/insightsengineering/teal.modules.clinical/blob/2b20a85b22f25f15f6808f745b4d1d5333b123f3/R/tm_g_ci.R#L484-L503

https://github.com/insightsengineering/teal.modules.clinical/blob/2b20a85b22f25f15f6808f745b4d1d5333b123f3/R/tm_g_km.R#L774-L793

Such hardcoded content in card$set_name() and card$append_text() has a great impact on the final ReportCard. If label in the Add Card module UI is not provided, then the card name is set to the one passed in card$set_name(). This is fine for cards that had empty labels - they will have a name populated by the developer of the teal module.

image

However card$append_text() like this https://github.com/insightsengineering/teal.modules.clinical/blob/2b20a85b22f25f15f6808f745b4d1d5333b123f3/R/tm_g_km.R#L778 and this https://github.com/insightsengineering/teal.modules.clinical/blob/2b20a85b22f25f15f6808f745b4d1d5333b123f3/R/tm_g_ci.R#L489 appends a content to the card. The card gets a hardcoded title, no matter what user specifies as a card label.

The solution

add_card_button_srv allows to specify card_fun with label parameter for card's title & content customization. This means, that you are able to use label shiny input and use it in your card_fun to pass the title. We are able to rewrite teal modules, so that they have default titles, if label is empty in Add Card module UI. When it is not empty, then the label is used as a name and as a title of the card - like in this commit https://github.com/insightsengineering/teal.modules.clinical/commit/5833c6c7f52aecac8da09b16da31e73fe5dffe07

With label

image image

Without the label

image so the default teal module name and title are used image

github-actions[bot] commented 1 year ago

badge

Code Coverage Summary

Filename              Stmts    Miss  Cover    Missing
------------------  -------  ------  -------  ----------------------------------------------------------------------------------
R/AddCardModule.R       144       2  98.61%   162, 199
R/Archiver.R             25       0  100.00%
R/ContentBlock.R         18       2  88.89%   38-44
R/DownloadModule.R      207      49  76.33%   89-95, 138, 163-168, 177-181, 184-188, 196-200, 203-207, 214-218, 221-225, 262-266
R/FileBlock.R            13       0  100.00%
R/NewpageBlock.R          2       0  100.00%
R/PictureBlock.R         30       2  93.33%   15, 79
R/Previewer.R           295      56  81.02%   183, 197, 199-202, 205, 208-216, 325-369
R/RcodeBlock.R           15       0  100.00%
R/Renderer.R            115      22  80.87%   61, 122, 130, 139, 141-162
R/ReportCard.R           77       4  94.81%   180, 219, 224, 245
R/Reporter.R             94       1  98.94%   254
R/ResetModule.R          55       0  100.00%
R/SimpleReporter.R       29       0  100.00%
R/TableBlock.R            9       0  100.00%
R/TextBlock.R            13       0  100.00%
R/utils.R               161      70  56.52%   7, 38-97, 99, 102-109, 163, 175-177
R/yaml_utils.R           81       2  97.53%   41, 239
TOTAL                  1383     210  84.82%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  -------
R/AddCardModule.R       +4      +1  -0.67%
TOTAL                   +4      +1  -0.03%

Results for commit: 29be0e6c28d148531e6576c21b12c5005b49b05a

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

github-actions[bot] commented 1 year ago

Unit Tests Summary

    1 files    18 suites   23s :stopwatch: 204 tests 204 :heavy_check_mark: 0 :zzz: 0 :x: 346 runs  346 :heavy_check_mark: 0 :zzz: 0 :x:

Results for commit 8858d32c.

:recycle: This comment has been updated with latest results.

m7pr commented 1 year ago

Is this in the scope of this feature? Can you make a separate branch and a separate fix, so this is cleaner and only focuses on what we initially discussed?

kartikeyakirar commented 1 year ago

Is this in the scope of this feature?

Not exactly but it affects report with Rcode is created but I have included this change in PR. so no worries.

https://github.com/insightsengineering/teal.modules.clinical/commit/5833c6c7f52aecac8da09b16da31e73fe5dffe07

The solution looks good and I recommend creating separate tasks for each package to facilitate easy tracking. This applies to teal.module.clinical, teal.modules.general, teal.modules.hermes, teal.osprey, and teal.goshawk. All these modules utilize the reporter.

m7pr commented 1 year ago

Hey @lcd2yyz and @donyunardi - pinging you here to double check you are ok with the proposed changes? In many teal modules that uses teal.reporter we had hardcoded card content titles. This PR allows the Card Name label to be passed as a title of Card Content. This is a big change, as this required so many modules to be changed in teal.modules.clinical - check this PR https://github.com/insightsengineering/teal.modules.clinical/pull/835

Is this the desired behavior? If yes, I will proceed with this change in other packages listed by @kartikeyakirar above teal.modules.general, teal.modules.hermes, teal.osprey, and teal.goshawk

m7pr commented 1 year ago

Hey @lcd2yyz and @donyunardi - we are ready to merge. Just waiting for your final green light.

m7pr commented 1 year ago

Following up on https://github.com/insightsengineering/teal.modules.general/issues/583 https://github.com/insightsengineering/teal.modules.hermes/issues/335 https://github.com/insightsengineering/teal.osprey/issues/228 https://github.com/insightsengineering/teal.goshawk/issues/240