Vignette covering changes of teal.reporter refactor
#1529
Conversation
| * Address limited customization capabilities of the previous `ReportCard` system, which was tightly integrated with `teal` module programming, making user modifications difficult. | ||
| * Resolve reproducibility challenges associated with the existing `teal.reporter` (see [teal.reporter issue #280](https://github.com/insightsengineering/teal.reporter/issues/280)). | ||
| * Incorporate solutions for enabling and disabling reporter functions globally and for single modules. | ||
| * Allow to customize report cards without modifying the `teal` module programming, thus enabling users to create their own report cards (and edit existing ones). | ||
| * Ensure that the new reporting system is lightweight and does not consume excessive disk space, especially for bookmarking purposes. | ||
| * Maintain or improve existing functionalities, including the ability to generate fully reproducible reports. | ||
| * Provide a more flexible and scalable design that can adapt to future needs (Editor module can be extended for new R classes). | ||
| * Export Editor functionality to allow users to edit report cards directly in the UI. | ||
| * Introduce a new feature to disable the reporter functionality for one or multiple `teal` modules. | ||
| * Improve the user experience by providing a more intuitive interface for report generation and customization (changes in Reporter Previewer UI). | ||
| * Ensure that the new system is compatible with existing `teal` modules and does not break any existing functionality. |
There was a problem hiding this comment.
Maybe somehow simplify this list of changes?
Can be used also to create NEWS.md entries
There was a problem hiding this comment.
This seems sensible, maybe changing to a feature list
1 example:
Ensure that the new reporting system is lightweight and does not consume excessive disk space, especially for bookmarking purposes.
to
Lightweight reporter system that reduces disk space usage (in particular with bookmarking)
| * **Technical Flow**: | ||
| 1. **Adding a `ReportDocument`**: When the `Add to Reporter` button (from `teal::add_document_button_srv`) is clicked within a `teal` module, a `ReportDocument` is created and added to the `Reporter`'s list of cards. | ||
| 2. **Previewing a Report**: Opening the "Report Preview(er)" tab triggers `reporter_previewer_srv`. This server function converts the `ReportDocument` elements into HTML for display using `toHTML`. | ||
| 3. **Downloading a Report**: Clicking "Download Report" in the previewer executes `report_render_and_compress`. This function uses the `Renderer` to generate an `.Rmd` file from the `ReportDocument` objects. The output is a `.zip` file containing the `.Rmd` report and a JSON file (for restoring the report). | ||
| 4. **Loading a Report**: Clicking "Load Report" clears the current report and restores it from the JSON file contained within an uploaded `.zip` archive. | ||
| 5. **Editing a `ReportDocument`**: Users can click an "Edit" button associated with a `ReportDocument` card in the previewer to modify its text elements. Custom edit functionalities can be specified for different content classes. | ||
|
|
There was a problem hiding this comment.
Do we need to explain the technical flow at all? Maybe for module developers this is helpful?
There was a problem hiding this comment.
Maybe as a standalone section or on a different user-facing vignette?
| * **UI Differences**: | ||
| * The only module-specific button for reporting is now "Add Card to Report". | ||
| * Global "Load Report", "Reset Report", and "Download Report" buttons are available in the Report Previewer. | ||
| * The download report encoding panel has been removed from the Report Previewer. It is now accessible via the "Download Report" button, which opens a modal dialog for encoding selection. This change was made to simplify the UI and reduce clutter. |
There was a problem hiding this comment.
Should we include print-screens of BEFORE and AFTER states?
There was a problem hiding this comment.
It would be good for the vignette, but it would need some maintenance over time.
Hopefully, there are no more big UI refactors
| * Ensure that the new reporting system is lightweight and does not consume excessive disk space, especially for bookmarking purposes. | ||
| * Maintain or improve existing functionalities, including the ability to generate fully reproducible reports. | ||
| * Provide a more flexible and scalable design that can adapt to future needs (Editor module can be extended for new R classes). | ||
| * Export Editor functionality to allow users to edit report cards directly in the UI. |
There was a problem hiding this comment.
Do we need a separate vignette for the Editor? Where we explain how to create your own editor_ui and editor_srv for custom classes?
| * Address limited customization capabilities of the previous `ReportCard` system, which was tightly integrated with `teal` module programming, making user modifications difficult. | ||
| * Resolve reproducibility challenges associated with the existing `teal.reporter` (see [teal.reporter issue #280](https://github.com/insightsengineering/teal.reporter/issues/280)). | ||
| * Incorporate solutions for enabling and disabling reporter functions globally and for single modules. | ||
| * Allow to customize report cards without modifying the `teal` module programming, thus enabling users to create their own report cards (and edit existing ones). |
There was a problem hiding this comment.
Should this point to a separate vignette for modify_reactive_output?
| * **Reduced storage size**: Being an S3 list, it avoids the overhead of storing methods and parent class environments associated with R6 objects, leading to smaller disk space usage, especially beneficial for bookmarking. | ||
| * **Flexible modifications**: `ReportDocument` supports easier reordering, subsetting, and editing of report content, both at the module development level and by the end-user in the Reporter Previewer tab. | ||
| * **Creation**: The primary function for creating a `ReportDocument` is `report_document()`. It can store various elements like characters (for text/markdown), data frames, plots, and code blocks. | ||
| * **Manipulation**: Standard list operations can be used. Additionally, `c.ReportDocument` and `[.ReportDocument` methods are provided. For robust editing, `edit_report_document()` allows modification and appending while preserving the `ReportDocument` class. |
There was a problem hiding this comment.
Still unsure if we should keep edit_report_document. I have a feeling it's not needed anymore
There was a problem hiding this comment.
That would keep the API a bit tighter, is it possible to completely replace the functions in edit_report_document?
Or does it need something like c(report_doc[1:2], "## Table 2", "yada", report_doc[3])
This sounds doable as our target audience with this feature are more experienced developers
| A significant focus of the refactor was to enhance reproducibility and the clarity of code presented in reports. | ||
|
|
||
| * **Reproducibility**: The new design with `ReportDocument` and the refactored download/upload process aims to ensure that reports are fully reproducible. | ||
| * **Code Display**: Enhancements to `teal.code::eval_code` and `teal.code::get_code` now support code labelling, improving how R code is captured and displayed in the generated reports. |
There was a problem hiding this comment.
This might not be true if we ever come up with a teal_data() + reporting integration and we abandon this code labelling.
| ``` | ||
|
|
||
| * **Modifying Module Output for Reports**: | ||
| * The introduction of `modify_reactive_output()` allows developers to programmatically alter or disable report cards generated by modules. This provides fine-grained control over what is reported. Further documentation and a dedicated vignette for `modify_report_output()` are planned: TODO. |
There was a problem hiding this comment.
Do we need a separate vignette for modify_report_output()?
There was a problem hiding this comment.
It's a bit covered at the end of the document
| # card$append_table(my_summary_table) | ||
| # card$append_src(code_string) | ||
|
|
||
| # New way |
There was a problem hiding this comment.
maybe point to PR in teal.modules.general?
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
|
|
||
| 6. **Return report_card as reactive in server**: |
There was a problem hiding this comment.
| 6. **Return report_card as reactive in server**: | |
| 5. **Return report_card as reactive in server**: |
| ``` | ||
| * This allows to apply the `modify_reactive_output()` function to the report card, enabling further customization and control over what is included in the final report. | ||
|
|
||
| 7. **No Need for `teal.reporter::simple_reporter_ui` and `teal.reporter::simple_reporter_srv`**: |
There was a problem hiding this comment.
| 7. **No Need for `teal.reporter::simple_reporter_ui` and `teal.reporter::simple_reporter_srv`**: | |
| 6. **No Need for `teal.reporter::simple_reporter_ui` and `teal.reporter::simple_reporter_srv`**: |
|
|
||
|
|
||
| 6. **Return report_card as reactive in server**: | ||
| * The `ReportDocument` should be returned as a reactive object in the server function of the module, wrapped in a names list under `report_card` name. |
There was a problem hiding this comment.
| * The `ReportDocument` should be returned as a reactive object in the server function of the module, wrapped in a names list under `report_card` name. | |
| * The `ReportDocument` should be returned as a reactive object in the server function of the module, wrapped in a named list under `report_card` name. |
| teal.reporter::code_chunk(code_string) | ||
| ) | ||
| }) | ||
| return(list(report_card = report_card)) |
There was a problem hiding this comment.
New name for report_card, or no name?
There was a problem hiding this comment.
I like the no name approach, maybe even accepting reactives (as well as list of reactives).. It might make the transition to teal_data easier.
|
This document mixes the transition with the previous reporter and new features. Would it make sense to make the document more general and keep notes after each section title on the differences? This way we can remove those afterwards and the document would still be up-to-date? @m7pr I think this goes on what you said in the meeting about this document be split in multiple (I'm working through the vignette itself, just wanted to discuss this while as this thought is hovering in every section) |
Yes @averissimo , but we also need some kind of a tutorial on how to migrate between old and new classes.
This would be great if you could help with that. Appreciate! |
|
|
||
| This document outlines a significant refactor of `teal.reporter` and accompanying enhancements in the `teal` framework for improved reporting functionalities. | ||
|
|
||
| The primary goals of enhanced reporting were to:` |
There was a problem hiding this comment.
| The primary goals of enhanced reporting were to:` | |
| The primary goals of enhanced reporting were to: |
| * Maintain or improve existing functionalities, including the ability to generate fully reproducible reports. | ||
| * Provide a more flexible and scalable design that can adapt to future needs (Editor module can be extended for new R classes). | ||
| * Export Editor functionality to allow users to edit report cards directly in the UI. | ||
| * Introduce a new feature to disable the reporter functionality for one or multiple `teal` modules. |
There was a problem hiding this comment.
Isn't this the same as line 28?
- Incorporate solutions for enabling and disabling reporter functions globally and for single modules.
| * Address limited customization capabilities of the previous `ReportCard` system, which was tightly integrated with `teal` module programming, making user modifications difficult. | ||
| * Resolve reproducibility challenges associated with the existing `teal.reporter` (see [teal.reporter issue #280](https://github.com/insightsengineering/teal.reporter/issues/280)). | ||
| * Incorporate solutions for enabling and disabling reporter functions globally and for single modules. | ||
| * Allow to customize report cards without modifying the `teal` module programming, thus enabling users to create their own report cards (and edit existing ones). | ||
| * Ensure that the new reporting system is lightweight and does not consume excessive disk space, especially for bookmarking purposes. | ||
| * Maintain or improve existing functionalities, including the ability to generate fully reproducible reports. | ||
| * Provide a more flexible and scalable design that can adapt to future needs (Editor module can be extended for new R classes). | ||
| * Export Editor functionality to allow users to edit report cards directly in the UI. | ||
| * Introduce a new feature to disable the reporter functionality for one or multiple `teal` modules. | ||
| * Improve the user experience by providing a more intuitive interface for report generation and customization (changes in Reporter Previewer UI). | ||
| * Ensure that the new system is compatible with existing `teal` modules and does not break any existing functionality. |
There was a problem hiding this comment.
This seems sensible, maybe changing to a feature list
1 example:
Ensure that the new reporting system is lightweight and does not consume excessive disk space, especially for bookmarking purposes.
to
Lightweight reporter system that reduces disk space usage (in particular with bookmarking)
|
|
||
| * **Motivation**: The `ReportCard` system faced limitations in customization, reproducibility, and storage efficiency. The new `ReportDocument` aims to overcome these by providing a simpler, more flexible, and lightweight approach to report generation. | ||
|
|
||
| * **`ReportDocument`**: |
There was a problem hiding this comment.
| * **`ReportDocument`**: | |
| * **`ReportDocument`** class: |
| * **Motivation**: The `ReportCard` system faced limitations in customization, reproducibility, and storage efficiency. The new `ReportDocument` aims to overcome these by providing a simpler, more flexible, and lightweight approach to report generation. | ||
|
|
||
| * **`ReportDocument`**: | ||
| * **Advantages**: |
There was a problem hiding this comment.
I think 1 and 3 are debatable, for instance the "Flexible modifications" are new features that could've been done in R6.
A simple title change might better reflect the whole section
| * **Advantages**: | |
| * **Advantages** & **New features**: |
| * **Reduced storage size**: Being an S3 list, it avoids the overhead of storing methods and parent class environments associated with R6 objects, leading to smaller disk space usage, especially beneficial for bookmarking. | ||
| * **Flexible modifications**: `ReportDocument` supports easier reordering, subsetting, and editing of report content, both at the module development level and by the end-user in the Reporter Previewer tab. | ||
| * **Creation**: The primary function for creating a `ReportDocument` is `report_document()`. It can store various elements like characters (for text/markdown), data frames, plots, and code blocks. | ||
| * **Manipulation**: Standard list operations can be used. Additionally, `c.ReportDocument` and `[.ReportDocument` methods are provided. For robust editing, `edit_report_document()` allows modification and appending while preserving the `ReportDocument` class. |
There was a problem hiding this comment.
That would keep the API a bit tighter, is it possible to completely replace the functions in edit_report_document?
Or does it need something like c(report_doc[1:2], "## Table 2", "yada", report_doc[3])
This sounds doable as our target audience with this feature are more experienced developers
| * **Technical Flow**: | ||
| 1. **Adding a `ReportDocument`**: When the `Add to Reporter` button (from `teal::add_document_button_srv`) is clicked within a `teal` module, a `ReportDocument` is created and added to the `Reporter`'s list of cards. | ||
| 2. **Previewing a Report**: Opening the "Report Preview(er)" tab triggers `reporter_previewer_srv`. This server function converts the `ReportDocument` elements into HTML for display using `toHTML`. | ||
| 3. **Downloading a Report**: Clicking "Download Report" in the previewer executes `report_render_and_compress`. This function uses the `Renderer` to generate an `.Rmd` file from the `ReportDocument` objects. The output is a `.zip` file containing the `.Rmd` report and a JSON file (for restoring the report). | ||
| 4. **Loading a Report**: Clicking "Load Report" clears the current report and restores it from the JSON file contained within an uploaded `.zip` archive. | ||
| 5. **Editing a `ReportDocument`**: Users can click an "Edit" button associated with a `ReportDocument` card in the previewer to modify its text elements. Custom edit functionalities can be specified for different content classes. | ||
|
|
There was a problem hiding this comment.
Maybe as a standalone section or on a different user-facing vignette?
| class(report_doc) | ||
| #> [1] "ReportDocument" | ||
|
|
||
| # Add content using c() - note ReportDocument should be the first argument for S3 dispatch |
There was a problem hiding this comment.
Some of the comments are too long, maybe we should make the code tighter?
| # card <- ReportCard$new() | ||
| # card$set_name("My Analysis Report") | ||
| # card$append_text("## Analysis Results") | ||
| # card$append_text("This is a summary of the analysis.") | ||
| # card$append_plot(my_ggplot) | ||
| # card$append_table(my_summary_table) | ||
| # card$append_src(code_string) |
There was a problem hiding this comment.
Why comment out? shouldn't this be still available?
| teal.reporter::code_chunk(code_string) | ||
| ) | ||
| }) | ||
| return(list(report_card = report_card)) |
There was a problem hiding this comment.
I like the no name approach, maybe even accepting reactives (as well as list of reactives).. It might make the transition to teal_data easier.
| @@ -0,0 +1,394 @@ | |||
| --- | |||
| title: "New reporting in teal" | |||
There was a problem hiding this comment.
I would love to keep our docs timeless and would recommend to avoid time-related phrases - even in the title. For example, if I open this document in a year or so - what does "new" actually mean?
Please just focus on explaining the current state as in typical technical manual. If we need to explain the deltas between the versions - that should be a separate release related entity (article, blog post, supplementary document etc.). As an example, we have done this in the past.
There was a problem hiding this comment.
also, title shouldn't differ from a filename.
|
@m7pr is this PR still active? |
This is a separate PR, so we don't pollute the original feature PR with a lot of new comments.