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

Vignettes: Support figures generated from code #55

Merged
merged 11 commits into from
Nov 7, 2023

Conversation

rempsyc
Copy link
Collaborator

@rempsyc rempsyc commented Oct 30, 2023

In this PR I attempt to solve the code-generated figures issue. I have decided to spend time on this because I think it is a pretty big issue that we will run into later with vignettes (turns out it is not unique to datawizard). My previous solution was not working properly.

Here I have tried using relative paths as much as possible since this is made possible and encouraged through the previous PR (#50). I feel like I have most of the pieces working: (a) images are moved from e.g., docs\articles\regression_files\figure-gfm to docs\articles\figures\regression_files\figure-gfm, and (b) md files paths are updated accordingly.

Etienne wrote in the code that:

However, docute and docsify need the paths in the .md files to
start with "articles/figures/" so I need to add "articles/"
just before "figures/" in the img paths, after compilation.

The original URLS in the md files look like this

<img src="../docs/articles/regression_files/figure-gfm/reg-1.png" width="1350" />

So I corrected them like this (added /figures):

<img src="../docs/articles/figures/regression_files/figure-gfm/reg-1.png" width="1350" />

But the images are still not detected properly it seems 🤔

no image

Not sure what the link should look like to be properly discovered. The files are in this location. Perhaps the paths need to be absolute rather than relative for the md files, but not sure how to change rmarkdown::render() to use absolute paths during rendering?

@rempsyc rempsyc marked this pull request as draft October 30, 2023 18:02
@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 1, 2023

Ok, so in the vignettes, two different types of image references are created, e.g.,

``` r
lavaan_reg(fit.reg, nice_table = TRUE, highlight = TRUE)
```
<img src="../articles/figures/regression_files/figure-gfm/reg-1.png" width="1350" />
``` r
plot(mtcars$mpg)
```
![](C:/github/lavaanExtra/articles/figures/regression_files/figure-gfm/unnamed-chunk-2-1.png)<!-- -->

First one now appears correctly, yeah! This is most likely because it uses relative paths because when rendering it actually takes a screenshot of html tables.

Second one doesn't work because it produces absolute paths. This is for any regular plot. Absolute paths in rmarkdown rendering seems to be caused by using the output_dir argument in rmarkdown::render(). Indeed, yihui writes,

Usually I don't recommend using output_dir or output_file. They are known to be problematic [...]. I believe the only reliable strategy is to render everything in the current directory, and move the output files later.

So this should be the solution next.

…al destination, add vignettes for internal testing

[skip ci] because currently breaks tests for mkdocs
@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 1, 2023

So in my new push, I realized that it would be simpler to simply render the vignettes directly inside the docs/articles/figures folder, move the vignettes one level, and keep the figures there. This way, the rmarkdown relative path works without having to deal with moving each image folder individually. In addition to greatly simplifying the code, I'm happy to say that works for datawizard, lavaanExtra, and altdoc. I also added two simple vignettes to altdoc to make testing a lot easier (i.e., no need to reinstall the package every time and test it with a separate package with many more vignettes...).

However, again, it only works when the URL uses the html form (<img src="), but not when using the markdown form (![]()). For example, from the two altdoc vignette examples below, only the bottom one works:

# Generated with: plot(mtcars$mpg)
![](articles/figures/vignette_testing_files/figure-gfm/plot-1.png)<!-- -->

# Generated with ![test-logo](hex-conductor.png)
<figure>
<img src="articles/figures/hex-conductor.png" alt="test-logo" />
<figcaption aria-hidden="true">test-logo</figcaption>
</figure>

Yet both paths start with articles/figures, so I wonder whether docsify and family support markdown references like this. The other explanation is that images need to be at the root of articles/figures, and not in subfolders like articles/figures/vignette_testing_files/figure-gfm/. If this is the case, it is a problem because it would mean that all figures would need to be at the same level, but sometimes unnamed chunks share the same figure names, so some would overwrite others.


Additionally, this rather substantial change in the vignette generation process broke the mkdocs tests, so I still need to make some adjustments in this regard.

@rempsyc rempsyc mentioned this pull request Nov 1, 2023
4 tasks
@vincentarelbundock
Copy link
Collaborator

it would be simpler to simply render the vignettes directly inside the docs/articles/figures folder, move the vignettes one level, and keep the figures there.

That sounds reasonable. Just make sure you copy the vignettes and not move them (as suggested above).

@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 5, 2023

I wasn't really clear in my previous message, but the steps are actually:

  1. Copy the entire vignettes folder from /vignettes to docs/articles/figures, so images that live in the /vignettes folder and referenced as such are also available using relative paths.
  2. Render the copied vignettes so figures naturally appear in the correct location, e.g., articles/figures/vignette_testing_files/figure-gfm/
  3. Move the copied vignettes one level, from docs/articles/figures to docs/articles/ so they are detected properly

So original vignettes are never moved or modified

@vincentarelbundock
Copy link
Collaborator

Sounds perfect

@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 5, 2023

Since this vignette is a substantial change, I integrated solution to #56 here (automatic integration of vignettes to navbar). It works! We just have to change the names to what we want, etc.

@vincentarelbundock
Copy link
Collaborator

cool, just let me know when you need a review.

@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 5, 2023

Second conclusion is that the markdown pathing:

# Generated with: plot(mtcars$mpg)
![](articles/figures/vignette_testing_files/figure-gfm/plot-1.png)<!-- -->

Actually works with altdoc::use_docute()!

use_docute

@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 5, 2023

It just doesn't work with altdoc::use_docsify(), which confirms my suspicions that the problem was with altdoc::use_docsify(). Further testing revealed that the following works: ![test](hex-conductor.png)

doscify_tag

But not ![](hex-conductor.png) without an image label:

doscify_no_tag

So for docsify, it seems rmarkdown images need to have labels...?

It also doesn't work with altdoc::use_mkdocs() but I am still investigating the cause. Since each page has its own index.html page embedded in a folder with the vignette name (e.g., altdoc\docs\site\articles\vignette_testing\index.html), it seems that page cannot find the image from the other folder (located higher at altdoc\docs\site\articles\figures).

@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 6, 2023

Ok, I think I fixed all errors, warnings, and notes! And, we have preliminary support for code-generated images!

I think I'm ready for you to review this and potentially merge it if all looks OK @vincentarelbundock.

Since this issue is becoming quite large, I would transfer the following remaining objectives in new smaller issues:

  • Rename update_vignettes() et al. for name consistency as discussed in update_vignettes() function #56
  • Add markdown support for docsify when images have no labels (e.g., should we attempt to add placeholder labels everywhere they are missing through regex? Issue with this workaround is that the labels appear under the image 😕)
  • Add support for mkdocs for images that are not detected properly right now (I think this should not be too hard to fix? Like moving images from altdoc\docs\site\articles\figures to their respective vignette folders, maybe?)

@rempsyc rempsyc marked this pull request as ready for review November 6, 2023 02:54
R/vignettes.R Outdated Show resolved Hide resolved
R/vignettes.R Outdated Show resolved Hide resolved
R/vignettes.R Show resolved Hide resolved
@vincentarelbundock vincentarelbundock merged commit 07df51b into main Nov 7, 2023
4 checks passed
@etiennebacher etiennebacher deleted the images_from_code branch November 7, 2023 07:22
@etiennebacher
Copy link
Owner

etiennebacher commented Nov 7, 2023

@rempsyc @vincentarelbundock please don't forget to update the NEWS in each PR if there are changes that directly affect the output or that should be visible to users. For example this PR introduces the altdoc_preview global option and previous ones exclude internal Rd files from being rendered in the Reference.

I have a documentation build failure in polars due to this PR (my bad, shouldn't rely on the dev version of altdoc), I'll investigate but a list of changes in the NEWS would be helpful.

@etiennebacher etiennebacher mentioned this pull request Nov 7, 2023
@vincentarelbundock
Copy link
Collaborator

Sorry, will do.

The global option was a "just for me" kind of thing. Do you want it to be user-visible and documented?

Could you tell us exactly what commands you run to update the polars website? They way we can test PRs against your workflow before merging PRs.

Sorry again for the conflicts.

@etiennebacher
Copy link
Owner

etiennebacher commented Nov 7, 2023

The global option was a "just for me" kind of thing. Do you want it to be user-visible and documented?

I just took this as an example because it was in the first changes I saw in this PR. If you ran into problems running preview() in VS Code for example then it's worth it, but otherwise I don't think it should be exposed.

Could you tell us exactly what commands you run to update the polars website? They way we can test PRs against your workflow before merging PRs.

You need to have a fork of polars (not necessarily up to date), then in the terminal:

Rscript -e 'altdoc::update_docs(custom_reference = "docs/make-docs.R")'
cd docs && python3 -m mkdocs serve

This should provide a link to the preview in the terminal.

You might need to build polars for this to work, and therefore have a Rust installation. I'll check anyway before each altdoc release.

@vincentarelbundock
Copy link
Collaborator

If you ran into problems running preview() in VS Code for example then it's worth it, but otherwise I don't think it should be exposed.

Yes, that was exactly my problem. I created a new PR with a documented argument in use_*() functions. This can be fixed manually or with a global option, since the signatures call getOption().

#60

@rempsyc
Copy link
Collaborator Author

rempsyc commented Nov 7, 2023

Pretty sure the polar problem is due to the use of the custom_reference argument, probably introduced by the quarto docs PR since I haven’t tested this argument before merging.

@etiennebacher
Copy link
Owner

The code for polars docs worked fine before this PR and fails after. To be more precise, this PR changed something in the YAML writing in mkdocs. In any case it should be solved by #58

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