-
Notifications
You must be signed in to change notification settings - Fork 335
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
Add support to html_vignette with as_is: true in pkgdown #1352
Conversation
Enable YAML option `set_null_theme` (logical) that works only if ``` pkgdown: as_is: true ```
I see that the test coverage test fails with error: But I cannot imagine how my changes could be related to the wrong version of covr on GH Actions: |
I'm not sure how to watch the progress of this commit but I'm commenting here so I get alerted to revert this saferactive/trafficalmr#14 |
Just a thought: it may be worth documenting how to use bookdown tags in vignettes. |
@Robinlovelace, do you mean (1) this: Lines 157 to 180 in fc092a8
Or (2) any section/topic from this book? (1) Is already implemented and (2), in my opinion, is out of the scope for pkgdown package. |
Could you please add a minimal Edit: You'd have to name this |
@GegznaV yes that is what I had in mind. I've made the point that you can cross reference tables and figures with a link the the bookdown package here - seem reasonable?: https://github.com/forked-packages/pkgdown/pull/1 |
|
I fully support this PR. Here's a great example of a html_vignette2 that can be built thanks to this work: https://saferactive.github.io/trafficalmr/articles/report1.html |
Why do we need to make this a parameter? Could we simply not reset the theme to true? (It's likely I'm missing something in the discussion; it would be very useful if someone could summarise (a) what precisely the problem is and (b) why this is the correct fix). |
I think that if |
By default, pkgdown builds all articles with If a vignette uses cross-references, autonumbering of figures and tables as well as other features from bookdown,
Thus Error messageFrom this example #1352 (comment) if the current version of pkgdown is used: > pkgdown::build_article("test-pkgdown-ok")
Reading 'vignettes/test-pkgdown-ok.Rmd'
Error: Failed to render RMarkdown
* Error in html_document(fig_width = fig_width, fig_height = fig_height, :
* formal argument "theme" matched by multiple actual arguments
Run `rlang::last_error()` to see where the error occurred. > rlang::last_error()
<error/rlang_error>
Failed to render RMarkdown
* Error in html_document(fig_width = fig_width, fig_height = fig_height, :
* formal argument "theme" matched by multiple actual arguments
Backtrace:
1. pkgdown::build_article("test-pkgdown-ok")
2. pkgdown:::render_rmarkdown(...)
Run `rlang::last_trace()` to see the full context. > rlang::last_trace()
<error/rlang_error>
Failed to render RMarkdown
* Error in html_document(fig_width = fig_width, fig_height = fig_height, :
* formal argument "theme" matched by multiple actual arguments
Backtrace:
x
1. \-pkgdown::build_article("test-pkgdown-ok")
2. \-pkgdown:::render_rmarkdown(...)
<error/callr_status_error>
callr subprocess failed: formal argument "theme" matched by multiple actual arguments
<error/callr_remote_error>
formal argument "theme" matched by multiple actual arguments
Backtrace:
x So it is at least strange when one can have some features in a vignette that is distributed with regular R documentation but cannot have the same features reproduced in the pkgdown version of the same documentation.
In this PR I do not question why certain pkgdown settings are used by default. I just provided a solution to a non-addressed problem and the solution:
Of course, It would be preferred if pkgdown supported |
Thanks for the explanation. Why can't we just not hardcode (Due to the way that pkgdown has to integrate the website template with the RMarkdown template, I don't think that's it's possible to support general RMarkdown documents, but I can make carefully considered changes for v. popular formats.) |
This But for HTML, I set up an experiment.
The result of For The result of To sum up, for the main format of HTML vignettes, I see no advantage of having a hardcoded |
I created an alternative PR #1371 to see if it passes the tests on CI. In the PR, the hardcoding theme to |
Superseded by #1371 |
This pull request addresses the parameter clash issue when the output format is
html_vignette
andas_is: true
(see #955) by introducing a new YAML parameterset_null_theme
which should be set tofalse
in this case. E.g.:You may install pkgdown with my changes
and try code:
in package test.pkgdown.zip to make sure that the changes do as what they are expected to do.
Solves #955.