insightsengineering / teal.modules.general

General Purpose Teal Modules
https://insightsengineering.github.io/teal.modules.general/
Other
9 stars 13 forks source link

[Feature Request]: Allow app dev to supply card function. #736

Open chlebowa opened 7 months ago

chlebowa commented 7 months ago

Feature description

Card functions being hard coded preclude the app dev and app user from shaping the report.

If there were an argument for a card function, one could customize their report without having to duplicate and modify a module.

Code of Conduct

Contribution Guidelines

Security Policy

donyunardi commented 7 months ago

Thanks @chlebowa
Definitely worth looking into this and it's great we have a head start with the PR that you raised.

Perhaps this feature can also encapsulate the request to skip reporter for a single module and disable them altogether.

We are interested in this and looking forward to collaborate with you on this. I've added this to the board for our next increment goal so we have time to review and discuss the design together.

chlebowa commented 6 months ago

Perhaps this feature can also encapsulate the request to skip reporter for a single module and disable them altogether.

Reporting is built into modules and cannot be controlled by the module constructor (e.g. tm_g_distribution). I can see setting the card function to NA as a signal to waive the built-in reporting but that would require other changes to the module. I don't think the issues overlap but perhaps this one can be a basis for the others.

I mentioned here a way of adding reporting to a module by a function call. If reporting were added explicitly, lie with with_reporter(tm_g_distribution(...), card_function) or with_reporter(tm_g_distribution, card_function)(...), dropping reporting would be trivial.

I've added this to the board for our next increment goal

Would that be June through July?

donyunardi commented 6 months ago

I can see setting the card function to NA as a signal to waive the built-in reporting but that would require other changes to the module. I don't think the issues overlap but perhaps this one can be a basis for the others.

That's exactly what I thought. I see that this can be the basis for other requirements.

Would that be June through July?

Correct, hope that's okay.

chlebowa commented 6 months ago

We'll make it work 👌

m7pr commented 4 months ago

Hey @chlebowa thank you for raising this feature request and for providing a propoasl implementation in this PR https://github.com/insightsengineering/teal.modules.general/pull/742.

Looks like @gogonzo and @pawelru expressed their opinion on the PRs (and also on the previous proposition with hydrating https://github.com/insightsengineering/teal.modules.general/pull/737).

However I don't think there is a clear agreement around the people involved in the review process on how to proceed forward. I think this comment from May needs more input https://github.com/insightsengineering/teal.modules.general/pull/742/files#r1600060687

@donyunardi would you mind scheduling something for the next week so we can handle and brainstorm when the team is back? It's a matter of whether we are satisfied from the temporary solution that @chlebowa provided, or we go with a bigger refactor as @gogonzo envisions. I would say this card is blocked until we make a decision

gogonzo commented 4 months ago

We (@pawelru , @chlebowa and @averissimo) discussed several propositions. Instead of having a function to interact with a ReporterCard methods we thought about markdown templates which probably will simplify the way how to build a report. App developer will be able to write a markdown file and add the content from the module through {{ object_from_the_module_environment }}. Anything put into {{ }} will be evaluated in a module environment so the values like plots, tables and text will be attached to the previewed and outputted document.

Example below is a alternative way of specifying ReportCard to callback function (please see the link)

https://github.com/insightsengineering/teal.modules.general/blob/54cd3917eded2ddde71ad8facc1d349b419c6c55/R/tm_g_distribution.R#L1279


{{ as.yaml(filter_panel_api$get_filter_state()) } # resolveDocParam.teal_slices

### Plot 
{{ 
  if (input$tabs == "Histogram") {
    dist_r()
  } else if (input$tabs == "QQplot") {
    qq_r()
  }
}} # resolveDocParam.ggplot

### Statistics table

{{ common_q()[["summary_table"]] }} # resolveDocParam.data.frame 

### Tests table

{{ tests_r() }}  # resolveDocParam.data.frame 

{{ 
  if (length(comment)) {
    sprintf("### Comment\n\n%s", comment)
  } 
}} # resolveDocParam.character

### R code

{{ teal.code::get_code(output_q()) }} # resolveDocParam.character

Above content will be parsed by internal teal process and dynamic content will be inserted in the document during a preview or download.


Another option is to create a Rmarkdown file which produces another R markdown file. So that code chunks can have asis = TRUE and produce other chunks.

asis

sprintf("```{r}\n%s\s```",
  if (input$tabs == "Histogram") {
    get_code(dist_q())
  else if (input$tabs == "QQplot") {
    get_code(qq_q())
  }
)
if (input$tabs == "Histogram") {
  dist_r() 
else if (input$tabs == "QQplot") {
  q_r()
}
chlebowa commented 4 months ago

@m7pr Thanks for looking into my PRs. #742 was the implementation for which we had tentatively agreed some time ago but we withhled further work until it could be put in a broader context of a teal.reporter refactor. Following our discussions, it appears the eventual solution will be different but I would leave the PR up until it is finalized.