Improve appearances svalvaro

[svalvaro]

Added changes to the report based on the Figma Design.

• Main changes: Make Subsections collapsible.
• Metric based risk assessment is now a table where some rows are collapsible containing multiple details (or nested tables).
• Added "Summary" UI element as per the design.

It's not totally polished yet, but a review would be great to know that I'm on the right track. After the running the demo report, it should generate the html like this:

I noticed that in the branch improve_appearances the PDF generation is broken (so it's the same in this branch), since I didn't attempt to fix that.

[llrs-roche]

Hi Alvaro, the check fails and when I run this locally I cannot generate a report:

pr <- package_report(
    package_name = "dplyr",
    package_version = "1.1.4",
    params = list(
      assessment_path = system.file("assessments/dplyr.rds", package = "riskreports"),
      image = "rhub/ref-image")
)
## 
## 
## processing file: validation_report_dplyr_v1.1.4.qmd
##   |................                |  49%                                      
## 
## Error in `is_risk_error()`:
## ! could not find function "is_risk_error"
## 
## Quitting from validation_report_dplyr_v1.1.4.qmd:257-257
##                                                                                                                                 
## Execution halted
## Error in `quarto::quarto_render()` at riskreports/R/reporter.R:93:7:
## ✖ Error running quarto cli.
## Caused by error:
## ! System command 'quarto.exe' failed
## Run `rlang::last_trace()` to see where the error occurred.

Is this ready for review?

I am not sure the library loaded in one environment is available on quarto, we might need to prefix the function with the package name so that it is found and used.

[svalvaro]

Hi @llrs-roche ,
Thanks for testing it out!
That is the error I messaged you on slack about when I checked out the branch improve_appearances. You can see how in your branch the pipeline is failing, so it's failing here as well.
I mentioned in the PR comment that I did not attempt to fix it, since from our conversation, I understood the priority was to work on the HTML output. (We could work on it on an independent PR.)

I noticed that in the branch improve_appearances the PDF generation is broken (so it's the same in this branch), since I didn't attempt to fix that.

The changes from this PR should be visible for the html output:

pr <- package_report(
    package_name = "dplyr",
    package_version = "1.1.4",
    params = list(
      assessment_path = system.file("assessments/dplyr.rds", package = "riskreports"),
      image = "rhub/ref-image"),
    output_format = "html"
)

[llrs-roche]

Great! Sorry I didn't realize you updated the first comment and went straight to review the code.
I'll keep this in mind and review the code and the html output. Thanks!

[llrs-roche]

I still get the same error message for the html output:

pr <- package_report(
+     package_name = "dplyr",
+     package_version = "1.1.4",
+     params = list(
+         assessment_path = system.file("assessments/dplyr.rds", package = "riskreports"),
+         image = "rhub/ref-image"),
+     output_format = "html"
+ )


processing file: validation_report_dplyr_v1.1.4.qmd
  |................                |  49%                                      

Error in `is_risk_error()`:
! could not find function "is_risk_error"

Quitting from validation_report_dplyr_v1.1.4.qmd:257-257
                                                                                                                                
Execution halted
Error in `quarto::quarto_render()` at riskreports/R/reporter.R:93:7:
✖ Error running quarto cli.
Caused by error:
! System command 'quarto.exe' failed
Run `rlang::last_trace()` to see where the error occurred.

I left some comments on the code. But my main concern is that moving to .callout-notes makes all the technical content purely informational. While I expect many visits to the report to be precisely by technical people wanting to compare why their environment/validation doesn't match the public report, so they will be checking the details of the information provided here.

[osenan]

Thanks for the review, I have addressed all comments except the comment about the .callout notes. Moreover, I refactored some code and added missing elements of the layout. Finally I styled all content so overall is almost matching 100% the original design.

Changes are:

• Add Rvalidation hub header
• Include link to docker image in title
• Style report in dark mode as in the design
• Refactor function to write cards
• Include unit tests for new functions

There are still some elements of style that do not match 100% the original design. There are specific content items that are not implemented, like the R CMD check. Finally we need to review that we do with the .callout-notes content and if we use another approach to reach similar behaviour. I need also to check why the R CMD check is failing.

Testing

To test the new package, first install it (assuming you are at the root of this project) with:
install.packages("./", repos = NULL)

Then create the report in html format:

  library(riskreports)

  options("riskreports_output_dir" = "/home/osenan/Documents/Appsilon/test_reports")

pr <- package_report(
  package_name = "dplyr",
  package_version = "1.1.4",
  params = list(
    assessment_path = system.file("assessments/dplyr.rds", package = "riskreports")),
  quiet = FALSE, # To silence quarto output for readability,
  output_format = "html"
)

[llrs-roche]

Many thanks for pushing this forward! The functions and test for the collapsible cards are great. They make it look the report pretty nice already!

Regarding the callout-notes, is there any other way to have collapsible content without them?

Regarding the R CMD check (riskreports is failing due to not declared suggested dependency on withr), we need to see how and when they are calculated, for the moment I would leave them on the body of the report and display if they are present, as this would mean however renders the report is already doing their own assessment and presumably know what they are looking for.

I see you modified how the report is handled from the package to were it is rendered and then to where it is saved. I wonder if it makes sense to use quarto command line option of output-dir (The latest version has some improvements related to that parameter).

There are a couple of visual improvements suggested in different comments.
However, one of the most important is how is the header reporting something and then the table below has a different content:

The license information should match wherever appears in the report.

Also note that some sections of the table have a button to expand but there is nothing in there.

Could these sections not be expandable if there is no extra information? I'm not sure if there is some information but it doesn't show up?

[llrs-roche]

The ">=2" v cannot be hard written, it depends on the package! Also I am not sure it is shown to the user.

[osenan]

You are right, my only problem is that I do not know to which value it refers this ">=2", is the version of the license or what it means?

[llrs-roche]

This is part of the license information that packages have. They can restrict that future versions of a given license apply or not (latest R has a tools::analyze_license that expand and verifies this). This is part of d_riskmetric$license and the >= section might be present or not (I might be just GPL-2)

[osenan]

Hi, thank you for your comments. I tried to address them al and think that now the report is better. Summary of changes:

• Fix wrong content like the cards or titles
• Put metrics in the table in the same order than in the design and included expandable sections only to the ones that contain extra content
• Added light design color

IMO there are 3 pending issues to merge all these changes:

• Analyze how to replace the callout notes with expandable output (in progress)
• Fix the R CMD check from CI
• Add the R CMD check output in the CI

[llrs-roche]

Many thanks for the updates! That was quick.

IMO there are 3 pending issues to merge all these changes:

• Analyze how to replace the callout notes with expandable output (in progress)
• Fix the R CMD check from CI
• Add the R CMD check output in the CI

I think the only blocking issue is the callout notes/expandable output. Edit: At the beginning I tried to do that via the knitr hood (knit_hooks on the setup code chunk).
Adding the R CMD check on the CI might be expensive and we'll need to coordinate with riskassessment app (and update/fix riskmetric). This will take time

[llrs-roche]

@osenan I've seen the changes related to the licenses, note that previous feedback was to link/explain what does it mean each license and I worked on a PR on #29. However, I'm not sure it is worth to require the latest R version for parsing the license and then adding some regex on top of it. We can maybe do the regex ourselves.

[osenan]

Hi, now I would say we really have 100% of the design as it was created. Last changes include:

• Add a lua filter to replace the .callout for a foldable output. This option looks better but it only works for print messages. For reactables it does not work, for plots it needs to be tested. However the filter could be modified (or a new one added) in case we would need filters.
• Add a js script to modify the output content of the foldable elements in order to include a copy button funcionality as the present in Quarto for code blocks
• Add fixes to the license and numbers. We will keep the R 4.5 because the funcionality will be used to always be updated for changes in licenses.

[llrs-roche]

It looks nice and works well in all formats!! Thanks!

Sorry to request more changes, you know what they say the effort for the last 20% is greater to get to the 80% correct.

On the pdf and html format I see some text that overflow the central area:

HTML (blocker)

pdf:

PDF (not a blocker)

Not sure if this is related to the quarto version used or something unrelated (OS used?)

On the (html) report build locally I see an error on the top of the "cards" and a message below them:

Error in loadNamespace(x): there is no package called 'riskmetric'

<cards>

Error in (function (index) : object 'x' not found

On the md and pdf report (via typst) format I see:

    Error in loadNamespace(x): there is no package called 'riskmetric'

    Error in data.frame(`Downloads 1 year` = as.character(r_riskmetric$downloads_1yr), : arguments imply differing number of rows: 1, 0

    Error: object 'simple_cards' not found

[osenan]

About the error, could it be that you do not have riskmetric installed?
I cannot reproduce it. I can try to create a snapshot test to make sure the report is correctly created in all formats.
About the overflow, I fixed in html. For pdf, it seems a case for the superlong line, because typst is addressing by default the other long lines wrapping it in newlines, but not this one. I suggest to solve it in a new issue, together with the new design of the pdf