-
Notifications
You must be signed in to change notification settings - Fork 1
custom report #39
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
base: main
Are you sure you want to change the base?
custom report #39
Conversation
Zhenglei-BCS
commented
Jul 10, 2025
- added a custom_report folder, will be merged into report folder
- added a custo _report function, will be mergerd into the main function.
Co-authored-by: Alvaro Sanchez <71273913+svalvaro@users.noreply.github.com> Co-authored-by: svalvaro <sanchezvillalba.alvaro@gmail.com> Co-authored-by: osenan <oriol@praenoscere.com>
Co-authored-by: osenan <oriol@praenoscere.com> Co-authored-by: Rabii Bouhestine <rabiibouhestine@outlook.com>
Summary of Changes from Original
|
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 introduces a new custom report feature—allowing light/dark logos and future color theming—by adding a custom_report folder, updating templates, and extending package_report() with new parameters.
- Added
logo_path,logo_path_dark,r_validation_logo_path,primary_color, andsecondary_colorto the report API - Updated Quarto templates and HTML/CSS to render dual‐mode logos and a footer
- Expanded documentation (
.Rdand vignettes) and included example R functions
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/articles/Generate-Customized-Report.Rmd | Spelling fix and updated usage example for custom logos |
| man/package_report.Rd | Documented new logo and color arguments |
| man/custom_package_report.Rd | Added custom report Rd and arguments; example usage mismatch |
| inst/report/package/top_page.html | Replaced single logo with dual‐logo container and dynamic title |
| inst/report/package/pkg_template.qmd | Injected new params and PDF fallback logo |
| inst/report/package/footer.html | Introduced a footer with R Validation Hub logo |
| R/utils.R | Duplicate is.empty definition |
| R/reporter.R | Copying logos and CSS removal logic |
Comments suppressed due to low confidence (3)
inst/dev/custom_report.R:10
- The
@paramblock documentsprimary_colorandsecondary_colorbut is missing@param logo_path_dark; please add documentation for thelogo_path_darkargument to keep the Rd in sync with the function signature.
#' @param primary_color Hex color code for primary styling (e.g., links, headers). Defaults to "#007BFF" (blue). Currently not applied; reserved for future customization.
man/custom_package_report.Rd:59
- The example calls
package_report()instead ofcustom_package_report(); update this to reflect the correct function name for the custom report.
pr <- package_report(
R/reporter.R:135
- The new logo‐copying and CSS removal logic isn’t covered by tests; consider adding unit or integration tests for
logo_path/logo_path_darkhandling to prevent regressions.
logo_to_remove <- c()
| is.empty <- function(x) { | ||
| is.null(x) || is.na(x) || !nzchar(x) | ||
| } |
Copilot
AI
Jul 10, 2025
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.
You’ve introduced is.empty which duplicates is_empty; consider consolidating to a single helper to avoid confusion and reduce code duplication.
| is.empty <- function(x) { | |
| is.null(x) || is.na(x) || !nzchar(x) | |
| } | |
| # Removed the duplicate is.empty function. Use is_empty instead. |
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.
Not sure if this is used somewhere else, @llrs-roche , I copied the original reporter.R and there is an is.empty function, and I put it to the utils.R, Copilot says it is redundant, can you confirm? I also kept the original function in orig_reporter.R, if you agree, we can also delete that.
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.
This is a real issue, I duplicated the functions. Maybe initially they did different things but now they have the same body. This should be fixed in all files of the package (not just on the ones you modified). So maybe we can left it as is and we address it later, or we can address it here for custom_report.R, orig_reporter.R, ...
We should replace is_empty by is.empty (in tests too) this would be the minimal diff.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
llrs-roche
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.
I like the idea of having a specific vignette for customizing the report.
However, how the packages provides this customization is not clear yet to me.
Adding a new function for each customization doesn't look sustainable in light of the _brand.yaml file (but that hasn't been tested). See the specific comments. In addition, this PR adds some folders inst/dev that I am not sure what they are.
This PR can be the base to write more robust code for customization.
| is.empty <- function(x) { | ||
| is.null(x) || is.na(x) || !nzchar(x) | ||
| } |
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.
This is a real issue, I duplicated the functions. Maybe initially they did different things but now they have the same body. This should be fixed in all files of the package (not just on the ones you modified). So maybe we can left it as is and we address it later, or we can address it here for custom_report.R, orig_reporter.R, ...
We should replace is_empty by is.empty (in tests too) this would be the minimal diff.
| params$package_name <- package_name | ||
| params$package_version <- package_version | ||
| params$image <- get_image_name(params) | ||
| params$logo_path <- logo_path | ||
| params$logo_path_dark <- logo_path_dark | ||
| params$primary_color <- primary_color | ||
| params$secondary_color <- secondary_color |
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.
I see that the arguments are then added to params. Good catch! I think it would make more sense to directly pass them as params, as I don't see any transformation or interaction before adding them to the parameters.
| # Placeholder for custom CSS or SCSS styling | ||
| # Note: Add custom styling here in the future. Options include: | ||
| # - Inline CSS by appending a <style> block to top_page_file with custom styles using params$primary_color and params$secondary_color. | ||
| # - Creating a separate custom CSS file in render_dir and ensuring it's referenced in the Quarto template YAML under format:html:include-in-header. | ||
| # - Dynamically updating SCSS files like custom.scss or custom_light.scss with color variables before rendering, if supported by the build process. | ||
| # Minimal CSS for dual-logo visibility in light and dark modes: | ||
| custom_style <- c( | ||
| "<style>", | ||
| " /* Dual-logo styling for light and dark modes */", | ||
| " .logo-container { position: relative; display: inline-block; }", | ||
| " .logo-light { display: block; }", | ||
| " .logo-dark { display: none; }", | ||
| " @media (prefers-color-scheme: dark) {", | ||
| " .logo-light { display: none; }", | ||
| " .logo-dark { display: block; }", | ||
| " }", | ||
| "</style>" | ||
| ) |
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.
This custom_style variable is not sustainable for all styles different organizations will like. The _brand.yaml file supported by quarto, allows to customize light and dark logos too:
logo:
small: logos/icon.png
medium:
light: logos/header-logo-light.png
dark: logos/header-logo-dark.png
large: logos/full-logo.svg
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.
Difficulties in using this approach:
- Quarto up to 1.6 did not clearly support dark mode branding (such as different logos or themes for dark mode) through the _brand.yml configuration. There are many ways to tweak it to work. Also it serves more for website project or project based, rather than single html file rendering. (Seems that the top.html need to be reconstructed in this case.)
- 1.7 supports dark and light both, but I don't have enough time to test it out yet as my RStudio does not pick up the newly installed quarto automatically but keeps using the quarto.exe under RStudio. I will have to try under Ubuntu environment instead. I will leave it here and get back to this after my vacation.
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.
Yes, with the reports we are on the edge of what quarto can provide. But I don't think we have other options that would have this. Rmarkdown can't provide this integration with multiple themes and the customizations we want as easily.
I too think that working with a single file is easier with Rmarkdown than with quarto. But ideally that drawback is reduced as the reports are generated for multiple packages.
About the problems with the quarto version picked on your computer I found a discussion. Which suggest this approach:
The version used in RStudio can also be forced by setting
RSTUDIO_QUARTOenv var
I hope this helps
…, something is wrong.
Make it more compatible with older packages
…utput directory to inst/dev/TMP_tests
…n your local machine.
39e28db to
5547d38
Compare