Skip to content

Closes #146: Interactive exploration of datasets vignette in website#227

Merged
Lina2689 merged 25 commits intomainfrom
146-documentation-allow-interactive-exploration-of-datasets-in-vignette
Feb 19, 2026
Merged

Closes #146: Interactive exploration of datasets vignette in website#227
Lina2689 merged 25 commits intomainfrom
146-documentation-allow-interactive-exploration-of-datasets-in-vignette

Conversation

@Gero1999
Copy link
Copy Markdown
Collaborator

@Gero1999 Gero1999 commented Jan 21, 2026

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.
Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the devel branch until you have checked off each task.

Implementation description

This PR:

  • Added a new vignette, preview-sdtm.Rmd, which provides an interactive, filterable preview of all datasets in the pharmaversesdtm package, displaying the first 50 rows of each dataset in collapsible panels using DT package.
  • Updated the site navigation in _pkgdown.yml to add a "Preview SDTM" link in the navbar, pointing to the new vignette.
  • Included the vignettes folder so they are ignored from the CRAN build

  • Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
  • Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
  • Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
  • If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
  • Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
  • Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
  • Address any updates needed for vignettes and/or templates
  • Update NEWS.md if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
  • Build pharmaversesdtm site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
  • Address or fix all lintr warnings and errors - lintr::lint_package()
  • Run R CMD check locally and address all errors and warnings - devtools::check()
  • Link the issue in the Development Section on the right hand side.
  • Address all merge conflicts and resolve appropriately
  • Pat yourself on the back for a job well done! Much love to your accomplishment!

Copy link
Copy Markdown
Contributor

@jimrothstein jimrothstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive! Ran fine in Rstudio cloud. I did not yet go through your code.

  • Change "Show XX entries" 100 is maximum in the drop-down. Change to 50 ? because actual limit is 50
  • On my screen, there is large left margin. Possible to remove?
  • Introductory text is fine.
  • Just nitpicking here:
  • "preview of the datasets" change to "preview of all datasets"
  • Text is all black and white; no indication user should click.
  • Several datasets are less than 50. Maybe I could add the number of rows, like
- ce_vaccine (44):  Clinical Events for Vaccine

Will go through your code now.

Image

@jimrothstein jimrothstein self-requested a review January 26, 2026 02:17
Copy link
Copy Markdown
Contributor

@jimrothstein jimrothstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code flows nicely - easy to follow.

  • need library() for DT, rlang?
  • I probably would have attempted either shiny or Github actions to create html.
    😄

@Gero1999
Copy link
Copy Markdown
Collaborator Author

Nice suggestions, I agree with all. And to respond your questions:

need library() for DT, rlang?

Good point. Needs to be added in DESCRIPTION

I probably would have attempted either shiny or Github actions to create html.

In principle this should already align with the GitHub action they already have in place (unless I missed something!)


Do you want me to start making these changes or you take care of it @jimrothstein ?

@jimrothstein
Copy link
Copy Markdown
Contributor

@Gero1999
I can do it, But late on Monday.
if you still have code open and want to finish it, pls go ahead.

@jimrothstein
Copy link
Copy Markdown
Contributor

jimrothstein commented Jan 27, 2026

fyi, this is from gemini:

description

More details:

For every package used in the vignette, there should be library call to be sure "package" isloaded.

library(package)

To insure the package is "installed" on user's system, the package should also be listed in DESCRIPTION.

The "law of the land" for R package development is the **Writing R Extensions** manual (the official CRAN documentation).

While it doesn't always use the word "must" in a legalistic sense, the requirement is enforced by the way R builds and checks packages. Because vignettes are compiled in a **fresh, isolated R session**, they lack access to your global environment or the package's internal namespace unless explicitly loaded.

---

## 1. The Official Documentation

The **Writing R Extensions** manual (Section 1.4: Vignettes) states:

> "The R code in the vignette will be executed in a new R session... it should be self-contained."

Because the session is "new," it starts with only the base R packages loaded. To use any other functions, you have to load the namespace.

## 2. Why "library()" is the standard

The **[R Packages (2e)](https://r-pkgs.org/vignettes.html)** book by Hadley Wickham and Jenny Bryan—which is the industry-standard interpretation of CRAN rules—clarifies this further:

> "A vignette is a long-form guide to your package... You should always start a vignette by loading your own package: `library(yourpackage)`."

### The "Self-Contained" Rule

If you run `R CMD check` on your package, R will attempt to "knit" or build your vignette.

* If your vignette uses a function like `filter()` but you **don't** have `library(dplyr)` in a code chunk, the check will throw an error: `Error: could not find function "filter"`.
* Therefore, the "documentation" is effectively the **R build system itself**; it won't let you pass the check unless the code is executable from start to finish.

---

## 3. How it looks in practice

Your vignette `.Rmd` (or `.qmd`) file should look like this structure:

```markdown
---
title: "Introduction to My Package"
output: rmarkdown::html_vignette
vignette: >
  %\VignetteIndexEntry{Introduction to My Package}
  %\VignetteEngine{knitr::rmarkdown}
  %\VignetteEncoding{UTF-8}
---

```{r, setup}
library(myPackage)  # Required to use your own functions
library(ggplot2)    # Required if you show plots in the vignette




## 4. One Exception: Double-Colon Notation
Technically, you don't *have* to use `library()` if you use the double-colon operator for every single function call (e.g., `dplyr::filter()`). 

However, this is highly discouraged in vignettes because:
1. It makes the code harder for users to read and copy-paste.
2. You **still** have to list those packages in your `DESCRIPTION` file under `Suggests` or `Imports`, otherwise the build will fail.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 27, 2026

Code Coverage

Package Line Rate Health
pharmaversesdtm 0%
Summary 0% (0 / 41)

@Lina2689
Copy link
Copy Markdown
Collaborator

@jimrothstein @Gero1999, The draft looks great! One suggestion is to categorize the Preview SDTM page by therapeutic area.

@Gero1999
Copy link
Copy Markdown
Collaborator Author

One suggestion is to categorize the Preview SDTM page by therapeutic area.

That is a great idea! I will use the specs yaml file instead then

@Gero1999
Copy link
Copy Markdown
Collaborator Author

Gero1999 commented Jan 28, 2026

I investigated a bit and for static pages seems to be better reactable due to how DT interacts with the DOM. So I changed that to what was originally suggested.

On the other hand, @jimrothstein I am not needing rlang, so I would say we don´t need in the DESCRIPTION, but lmk in case I am missing something!

Also if you feel everything is ready perhaps we can open the PR and ask for additional reviews

@jimrothstein
Copy link
Copy Markdown
Contributor

@Gero1999
Looks good, but I can not get it to render in posit cloud.

Starts to render as html, but stops at first chunk (echo = FALSE); no error.
Probably I am doing something wrong.

@Lina2689
Copy link
Copy Markdown
Collaborator

@Gero1999 Looks good, but I can not get it to render in posit cloud.

Starts to render as html, but stops at first chunk (echo = FALSE); no error. Probably I am doing something wrong.

there is some issue, check below error

image

@jimrothstein
Copy link
Copy Markdown
Contributor

@Lina2689 - good catch, but I think better if @Gero1999 can do this.

@Gero1999
Copy link
Copy Markdown
Collaborator Author

image

I might have missed to load the package. I will give it a quick look and see for an easy fix

@Gero1999
Copy link
Copy Markdown
Collaborator Author

hopefully now is fixed! otherwise I will give it a deeper look later

@Lina2689
Copy link
Copy Markdown
Collaborator

hopefully now is fixed! otherwise I will give it a deeper look later

Looks good now. Just make sure the therapeutic area sorting order is maintained.

@Gero1999 Gero1999 marked this pull request as ready for review January 28, 2026 15:04
Copy link
Copy Markdown
Collaborator

@Fanny-Gautier Fanny-Gautier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool Vignette! Well done and Great addition to the package!

2 other minor comments:

  • Is it possible to enlarge the columns to get the variables name on 1 line only?
Image
  • Is this warning message expected? Either fixe it with the suggestion or rename it accordingly.
Image

@Fanny-Gautier
Copy link
Copy Markdown
Collaborator

Also - way cool!!

Just musing - would it be easier to have as a standalone website for both sdtm and adam that is linked to this pkgdown site? That way the maintenance/views could be experimented with updated and not depend on a release??

Unsure if this would get release when available or have to wait for a pkg release...feels like a great idea to get out the door quickly :)

The plan was to issue an ad‑hoc release in the spring to include the microbiology data, the new vignette will therefore be available at that time as well.

@bms63
Copy link
Copy Markdown
Collaborator

bms63 commented Jan 30, 2026

Also - way cool!!
Just musing - would it be easier to have as a standalone website for both sdtm and adam that is linked to this pkgdown site? That way the maintenance/views could be experimented with updated and not depend on a release??
Unsure if this would get release when available or have to wait for a pkg release...feels like a great idea to get out the door quickly :)

The plan was to issue an ad‑hoc release in the spring to include the microbiology data, the new vignette will therefore be available at that time as well.

if the package isn't changing, you could delete the release and do a "new release" with the vignette and that will trigger the build of website with the vignette. be so cool to have it out before spring time! it is such a neat addition.

Copy link
Copy Markdown
Collaborator

@Lina2689 Lina2689 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

above link is not working

Image

Gero1999 and others added 3 commits January 30, 2026 20:54
@Gero1999 Gero1999 marked this pull request as draft January 31, 2026 06:33
@Gero1999
Copy link
Copy Markdown
Collaborator Author

As requested by @Fanny-Gautier I applied the suggestions and I also made other table display changes to improve readability
As requested by @Lina2689 via chat I incremented version and added authorship so it also applies for whenever #218 is merged

Regarding this:

Image above link is not working Image

The thing is that we cannot make sure this link works until we merge and deploy the page (I am using the link is supposed to hold the website page). Perhaps is cleaner if we open a different PR after merging this one in order to do this. Or we can keep what it is and correct after merging otherwise. I leave it to your criteria

Hopefully I did not miss anything, but please feel free to point out any potential improvement or missing aspect!

Copy link
Copy Markdown
Collaborator

@Fanny-Gautier Fanny-Gautier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments to implement, plus please tick all the boxes in the checklist, and I believe it will be ready to go!
The link issue on the main page will be resolved once the PR is merged into main and the updated website is available online.
Thank you for providing such an amazing vignette!

NEWS.md Outdated

- The reference page has been updated to categorize datasets by therapeutic areas. (#204)


Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete extra space.

Copy link
Copy Markdown
Collaborator

@Fanny-Gautier Fanny-Gautier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting order to implement and it will be ready to go! Great job here too, Thank you @Gero1999 !

Copy link
Copy Markdown
Collaborator

@Lina2689 Lina2689 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Copy Markdown
Collaborator

@Fanny-Gautier Fanny-Gautier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thank you!

@Gero1999
Copy link
Copy Markdown
Collaborator Author

Thanks for the reviews @Fanny-Gautier @Lina2689, I am not allowed to merge but feel free to do so if you feel is ready to go!

image

@Lina2689
Copy link
Copy Markdown
Collaborator

@manciniedoardo Could you please approve the PR? I’ll merge it right after approval.

@Lina2689 Lina2689 merged commit 8c7d71e into main Feb 19, 2026
16 checks passed
@Lina2689 Lina2689 deleted the 146-documentation-allow-interactive-exploration-of-datasets-in-vignette branch February 19, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documentation: Allow interactive exploration of datasets in referrence pages/vignette

6 participants