-
-
Notifications
You must be signed in to change notification settings - Fork 15
Adds rmarkdown module
#944
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
Conversation
|
Marked as ready to review to start the discussion |
Code Coverage SummaryDiff against mainResults for commit: d2df067 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
gogonzo
left a comment
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.
Shouldn't both documets be reproducible?
- document downloaded from tm_rmarkdown
- document downloaded via reporter
|
Key questions:
|
|
Would it be possible to use the reporter function to download a rendered
document? Ideally the user would have the choice between html and pdf.
What do you think?
…On Fri, Nov 7, 2025 at 8:38 AM Dawid Kałędkowski ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In R/tm_rmarkdown.R
<#944 (comment)>
:
> +
+ if (allow_download) {
+ out_data <- eval_code(
+ q_r(),
+ paste(
+ sep = "\n",
+ sprintf("## R Markdown contents are generated from file, please download it from the module UI."),
+ sprintf("# rmarkdown::render(%s, params = params)", shQuote(basename(rmd_file), type = "cmd"))
+ )
+ )
+ ***@***.*** <- FALSE # manual change verified status as code is being injected
+ }
+
+ teal.reporter::teal_card(out_data) <- c(
+ teal.reporter::teal_card(out_data),
+ rendered_html_r()
We could keep it as a single markdown object
I think its good to have a single markdown string because anyway at the
end (before download) we will concatenate everything to the single file.
Only editor will show one combined text-field for this element (i think it
is not a big issue so far)
the current approach was meant to reduce duplicate rendering
This is a fair reason. I feel (not a strong opinion) that if you render
markdown using rmarkdown::render and includeMarkdown the speed should be
similar to rmarkdown::render to html. rmarkdown renders in two stages to
html, first by converting Rmd -> md and then pandoc renders from md ->
html.
The biggest issue with including html is that html can't be converted to
pdf, otf etc.
—
Reply to this email directly, view it on GitHub
<#944 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJUWWERCKCNM7NA66YG6VVT33REA3AVCNFSM6AAAAACLHVPL3CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTIMZSGA3TSNRYGU>
.
You are receiving this because you were mentioned.Message ID:
<insightsengineering/teal.modules.general/pull/944/review/3432079685@
github.com>
|
|
@StefanThoma that's one of our goals with this PR. It's now possible for HTML output, the other formats are a WIP, but I believe we'll make it happen |
Unit Tests Summary 1 files 24 suites 15m 33s ⏱️ For more details on these failures and errors, see this check. Results for commit d2df067. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 82ed11b ♻️ This comment has been updated with latest results. |
osenan
left a comment
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.
Is it possible to create unit tests for srv_markdown? With testServer I think it has complex enough server logic to implement them.
Also check if other exported funcions we can create uni test.
I tested the app, based on examples. It looks ok:

But the markdown text is written by he teal_developr right? Are not the users who can add/remove markdown text
|
@osenan I've added tests and I think I addressed your comments. Let's have a new round of review :-) |
osenan
left a comment
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 for the tests. I have run them locally and they pass. They do not pass in CI due to e2e tests.
They are also very useful. There is a lintr error, we could ignore the lint for names in that line.
Signed-off-by: André Veríssimo <[email protected]>
Pull Request
Thanks to @StefanThoma's for working on the first prototype
Changes description
extra_transformparameter to add "generic decorators" to the top of the UITo discuss
.Rto get all the R code from the report and add it to the code of theqenv?markdownwhen rendering doesn't print title nor authortealiframedoesn't extend full height and then creates strange reportstransformersto add extra input / data manipulation(topics below added from conversation with @gogonzo )
teal_reportcontain markdown elements?teal_dataevaluatermarkdown::render()and include this to@codermarkdown::render(envir = <qenv>)instead ofparams = list()?App (same 2nd one in example, but with
download = TRUE)