-
-
Notifications
You must be signed in to change notification settings - Fork 2
📑 Reporter Refactor: Adds support for new teal_report and teal_card class
#353
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
Code Coverage SummaryDiff against mainResults for commit: e7d0f45 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary1 files 1 suites 2m 1s ⏱️ Results for commit ed95681. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 32757ee ♻️ This comment has been updated with latest results. |
…ing/teal.goshawk into teal_reportable
Signed-off-by: Marcin <[email protected]>
Signed-off-by: Marcin <[email protected]>
Signed-off-by: Marcin <[email protected]>
Signed-off-by: Marcin <[email protected]>
Signed-off-by: Marcin <[email protected]>
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.
All examples could save and load the report. Good job!
Sorry for the late review!
Probably not addressable here but when rendering the reports for the examples when saving and loading a report I see:
Quitting from report.Rmd:58-60 [unnamed-chunk-5]
Render document error: Error in `gzfile()`:
! cannot open the connection
The lines where the unnamed chunk 5 is change depending on the module but it remains stable.
However, I could see the report loaded correctly.
|
@llrs-roche this got fixed in herehttps://github.com/insightsengineering/teal.reporter/pull/385 |
teal_report and teal_card class
Signed-off-by: Marcin <[email protected]>
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.
The reports look good 👍
I think this still needs some changes:
teal.reporterversion bump in DESCRIPTION- use dimensions of image in browser to set reporter plot (1 comment, but applies to all)
There's a question about the constraints section that was completely removed.
Signed-off-by: André Veríssimo <[email protected]>
…ing/teal.goshawk into teal_reportable
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.
spaghettiplot is not working
All the others are great! 🥳
Hey, I hope you meant tm_g_gh_density_distribution_plot. I just fixed it and pushed the updates |
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.
Pull Request Overview
This PR refactors the reporter functionality to support new teal_report and teal_card classes, replacing the legacy "Add to reporter" implementation. It systematically removes the old reporter UI buttons and server logic from all goshawk plot modules and introduces a new teal_reporter integration approach.
- Removes legacy reporter implementation across all plot modules
- Adds new utility functions for setting chunk attributes and dimensions on
teal_cardobjects - Updates modules to return
teal_reportobjects with embedded plot cards and metadata
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| R/utils.R | Adds utility functions for managing teal_card attributes and plot dimensions |
| R/tm_g_gh_*.R | Refactors all plot modules to use new reporter system, removes legacy UI/server code |
| man/*.Rd | Updates documentation and removes legacy example code patterns |
| DESCRIPTION | Updates version dependencies for companion packages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Awesome!
Tested with
Companion PRs:
teal_reportandteal_cardclass teal#1541{teal}module returns ateal_reportobject that extends fromteal_datateal.code#255{teal}module returns ateal_reportobject that extends fromteal_datateal.data#370teal_report,teal_cardclass and revamps reporter UI teal.reporter#331teal_reportandteal_cardclass teal.modules.general#884Changes description
teal_reportobject on every module