CMAP-REPOS / cmapplot

Set of templates and themes to apply CMAP graphics standards to R products.
https://cmap-repos.github.io/cmapplot/
Other
13 stars 1 forks source link

Vignette image fix #132

Closed dlcomeaux closed 3 years ago

dlcomeaux commented 3 years ago

Preliminary PR to fix the vignette margin image problem. I'm not sure if this currently works - it builds it locally, but the old code did as well. @nmpeterson - I think you had previously shared it was possible to do a test build of the site, right (but couldn't find where)? How do we do that again?

nmpeterson commented 3 years ago

Every time a pull request is created (or updated), the pkgdown action will attempt to build the website into the gh-pages-test branch. Unlike the gh-pages branch, the test site is not actually served as a hosted website anywhere, but you can browse the files on GitHub.com, or clone the branch to your PC and open the website from File Explorer.

dlcomeaux commented 3 years ago

Ah ok @nmpeterson - when I clone the current version, which appears to be based on the commit, the HTML files work when I open them in my browser. However, that also appears to be the case for previous code (I downloaded an earlier set of code from the test branch). So, I'm not sure whether this has had any impact, or how to test it except pushing it into the main branch. This is definitely not a part of the repo I particularly understand, though, so, open to other ideas.

nmpeterson commented 3 years ago

I finally had some time to look at this in some detail. Everything looks good to me. Vignettes seem to load the images correctly in both the pkgdown build and the R install (i.e. pkgdown::install(build_vignettes=TRUE) => library(cmapplot) => browseVignettes("cmapplot")).

dlcomeaux commented 3 years ago

OK - @nmpeterson are you comfortable with me merging into 1.2 then?

nmpeterson commented 3 years ago

I'm fine with it. Not sure if @matthewstern wants to review first.

matthewstern commented 3 years ago

I'm ok with trying this although it's worth noting that previewing here does not work: https://raw.githubusercontent.com/CMAP-REPOS/cmapplot/gh-pages-test/man/figures/margins1.PNG

nmpeterson commented 3 years ago

@matthewstern GitHub seems to have case-sensitive URLs (at least for the filename portion). This works: https://raw.githubusercontent.com/CMAP-REPOS/cmapplot/gh-pages-test/man/figures/margins1.png

matthewstern commented 3 years ago

@nmpeterson you have, I believe, identified the actual underlying problem. The currently live pkgdown website links to https://cmap-repos.github.io/cmapplot/man/figures/margins1.PNG but the file that exists is https://cmap-repos.github.io/cmapplot/man/figures/margins1.png. This is despite the fact that the vignette itself links to the lowercase png: <img src="./../man/figures/margins1.png" width="100%" style="display: block; margin: auto;"/>

This suggests to me that the fix here just merged is actually not going to work. I would guess this error came to pass originally because of a pkgdown update that is for some reason capitalizing the file extension. Identifying the pngs as resources as @dlcomeaux has done here might be part of the solution, but I don't think it's going to take us all the way.

matthewstern commented 3 years ago

@dlcomeaux I'm wondering the simplest solution here may actually be to rename the files in the github directory to .PNG, and edit the reference in the vignette so that it also looks for a capitalized extension. Theoretically, the external files code you added would no longer be necessary (pkgdown suggests that automatic detection should work, and resource_files is just a backup).

Would you be willing to give this a try?

nmpeterson commented 3 years ago

@dlcomeaux before you try @matthewstern's suggestion, let's review my new PR (#133). The repo already had the files as uppercase. I renamed them to lowercase using command line git, so I believe no further changes should be necessary.

dlcomeaux commented 3 years ago

Thanks all - just getting to this, seems like everything is in order. Thanks!