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

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

Open
3 tasks done
chlebowa opened this issue Apr 19, 2024 · 7 comments
Open
3 tasks done

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

chlebowa opened this issue Apr 19, 2024 · 7 comments
Labels
blocked core enhancement New feature or request

Comments

@chlebowa
Copy link
Contributor

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

  • I agree to follow this project's Code of Conduct.

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines.

Security Policy

  • I agree to follow this project's Security Policy.
@chlebowa chlebowa added the enhancement New feature or request label Apr 19, 2024
@donyunardi
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

We'll make it work 👌

@kumamiao kumamiao moved this to Todo in teal Roadmap Jun 13, 2024
@m7pr
Copy link
Contributor

m7pr commented Jul 10, 2024

Hey @chlebowa thank you for raising this feature request and for providing a propoasl implementation in this PR #742.

Looks like @gogonzo and @pawelru expressed their opinion on the PRs (and also on the previous proposition with hydrating #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

@m7pr m7pr added the blocked label Jul 10, 2024
@gogonzo
Copy link
Contributor

gogonzo commented Jul 12, 2024

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)

card_fun <- function(comment, label) {

{{ 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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked core enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

4 participants