Skip to content
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

[r] restore vignettes to docsite (in static HTML form) #583

Merged
merged 23 commits into from
Aug 2, 2023

Conversation

mlin
Copy link
Contributor

@mlin mlin commented Jun 27, 2023

  • Move the vignettes into vignettes_/ (with the underscore so that the automated R package build process ignores it)
  • Add script to build the Rmarkdown files into static HTML files, which are checked into git alongside
  • Add a step to the docsite-build-deploy.yml workflow to copy those HTML files into the docsite
  • Add a vignette index markdown page to the docsite, linking to the vignette HTML files (and add this index to the TOC)

I could not find a way to make pkgdown directly incorporate the static HTML vignettes (unanswered discussions: 1 2), so here w add them separately and use the pkgdown site just as the API reference.

@mlin
Copy link
Contributor Author

mlin commented Jun 27, 2023

@ebezzi Welcome any early feedback on this diff. I have not actually tested the docsite changes, yet.

@mlin mlin marked this pull request as ready for review June 30, 2023 16:51
@mlin
Copy link
Contributor Author

mlin commented Jun 30, 2023

Tested the Sphinx build locally; I think this will work as long as I correctly predicted the URLs where the vignette HTMLs will end up.

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #583 (9b51a48) into main (ac2d4f9) will increase coverage by 0.01%.
Report is 8 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
+ Coverage   87.05%   87.06%   +0.01%     
==========================================
  Files          65       65              
  Lines        4473     4493      +20     
==========================================
+ Hits         3894     3912      +18     
- Misses        579      581       +2     
Flag Coverage Δ
unittests 87.06% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@ebezzi ebezzi left a comment

Choose a reason for hiding this comment

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

Thanks Mike! This looks great. My only concern is that we don't really have a way to test this end to end. In case there is some kind of issue with the links, we should be able to release a minor version with the fixes relatively easily. @pablo-gar OK with that?

In case we want a more robust testing pipeline for the docsite, let me know and I'll come up with a proposal.

docs/index.rst Outdated
cellxgene_census_docsite_r_tutorials.md
R API <https://chanzuckerberg.github.io/cellxgene-census/r/reference/index.html>
Copy link
Member

Choose a reason for hiding this comment

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

Where does the reference come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebezzi thanks for highlighting this -- one thing I've noticed repeatedly with pkgdown landing pages (such as ours) is that it's super easy to overlook the "Reference" link on the top bar, since all the other links on the page are blue underlined except for that one. So I find myself often getting mentally stuck on the landing page (which is just a copy of README.md) for a few seconds, until I remember to look for that link.

With the static HTML vignettes suggested here, we're only using the pkgdown site as the API reference, so I thought I'd just link directly and skip that roadbump. Open to alternative ideas though. cc @pablo-gar

@pablo-gar pablo-gar self-requested a review July 8, 2023 00:43
Copy link
Contributor

@pablo-gar pablo-gar left a comment

Choose a reason for hiding this comment

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

I'd like to discuss the design further, and I have a few questions.

@mlin Currently with this design we end up with the following:

image
  • And clicking on any of the tutorial links will lead to opening a static html with nothing but the vignette -- no top bar or side bar:
image
  • And the R doc-site will not have the "Articles" page either (where vignettes are usually placed):
image

If all of the above are correct then we need some refinement to produce a more consistent and fluid reading experience. Let's chat over call to ideate on this

@mlin mlin mentioned this pull request Jul 10, 2023
@mlin
Copy link
Contributor Author

mlin commented Jul 10, 2023

Next step: build the whole pkgdown site (including vignettes) offline, checked in as static HTML; then copy that whole pkgdown tree into the docsite as we're building it in github actions. And make sure the procedure for this is documented in the readme.

@mlin
Copy link
Contributor Author

mlin commented Jul 17, 2023

@pablo-gar @ebezzi Revised this as we discussed, so we run pkgdown locally including the vignettes and check it all into git, to be copied in during the docsite build workflow. Please look over!

@pablo-gar pablo-gar self-requested a review July 21, 2023 21:19
Copy link
Contributor

@pablo-gar pablo-gar left a comment

Choose a reason for hiding this comment

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

Small comments here and there, please address.

Please merge #406 onto this

And please make this the _pkgdown.yml

url: ~
template:
  bootstrap: 5
  includes:
    in_header: <script defer data-domain="chanzuckerberg.github.io/cellxgene-census" src="https://plausible.io/js/script.js"></script>

home:
  sidebar: FALSE
      
navbar:
  structure:
    left: [docsite, reference, articles]
  components:
    docsite:
      text: Main doc-site
      href: https://chanzuckerberg.github.io/cellxgene-census/
        
reference:
- title: Open/retrieve Census data
  contents:
    - open_soma
    - new_SOMATileDBContext_for_census
    - download_source_h5ad
    - get_source_h5ad_uri
- title: Export slices of data
  contents:
    - get_seurat
- title: Feature presence matrix
  contents:
    - get_presence_matrix
- title: Versioning of Census builds
  contents:
    - get_census_version_description
    - get_census_version_directory

articles:
- title: Analysis
  navbar: "Learn about the data do analysis"
  contents:
    - comp_bio_census_info
    - comp_bio_summarize_axis_query
- title: API
  navbar: "cellxgene.census capabilities"
  contents:
    - census_query_extract
    - census_datasets
    - census_dataset_presence

api/r/cellxgene.census/README.md Outdated Show resolved Hide resolved
cd api/r/cellxgene.census
Rscript -e 'pkgdown::build_site()'
cd ../../../
# Copy R pkgdown docs (static HTML checked into git)
mkdir -p docsite/r
cp -r api/r/cellxgene.census/docs/* docsite/r/.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow -- this is just copying the static files, what would be the failure mode of concern? (That stated, we obviously haven't tried this workflow yet since it actually deploys the live docsite -- first attempt certainly could get interesting)

@@ -12,7 +12,7 @@ knitr::opts_chunk$set(
collapse = TRUE,
comment = "#>"
)
options(width = 88)
options(width = 88, max.print=256)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are some notebooks max.print=256 and others max.print=100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was just subjective based on reviewing the built vignettes and what would look good. If we print "wide" dataframes with many columns, they're shown in a wrapped form, so they can get super long down the page and it helps to use a lower max.print in those cases. But not all of the vignettes include such wide dataframes.

@pablo-gar pablo-gar merged commit 4081aec into main Aug 2, 2023
14 checks passed
@pablo-gar pablo-gar deleted the mlin/r-restore-vignettes branch August 2, 2023 02:09
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.

3 participants