-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
expand repo links #1447
base: main
Are you sure you want to change the base?
expand repo links #1447
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool ! Thanks.
Maybe we could some test like with
bookdown/tests/testthat/test-bs4_book.R
Lines 70 to 83 in cedaac9
test_that("bs4_book() repo specification works - GitLab", { | |
skip_if_bs4_book_deps_missing() | |
book <- local_bs4_book( | |
output_options = list( | |
repo = "https://gitlab.com/hadley/ggplot2-book" | |
) | |
) | |
html <- xml2::read_html(file.path(book, "_book", "index.html")) | |
expect_equal( | |
xml2::xml_attr(xml2::xml_child(xml2::xml_find_first(html, "//a[@id='book-repo']")), "class"), | |
"fab fa-gitlab" | |
) | |
}) |
DESCRIPTION
Outdated
@@ -78,6 +78,7 @@ Suggests: | |||
rsconnect (>= 0.4.3), | |||
servr (>= 0.13), | |||
shiny, | |||
svglite, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why svglite is added ?
Missing suggest ? I don't see where it is used in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to pass the check, since it's used in inst/examples/index.Rmd
, otherwise it doesn't let me make a PR... the "build and deploy book" one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So missing suggest - thanks !
As this is for rendering this book only (bookdown demo works ok), I looked into it closer. I believe this is due to change in knitr where svglite is called (yihui/knitr@4300e49)
And we are indeed asking for svglite in our gitbook format for this book
bookdown/inst/examples/_output.yml
Lines 1 to 2 in cedaac9
bookdown::gitbook: | |
dev: svglite |
So this is really a dependency only for the book. We do plan its installation
bookdown/inst/examples/index.Rmd
Lines 34 to 36 in cedaac9
lapply(c('DT', 'formatR', 'svglite', 'rticles'), function(pkg) { | |
if (system.file(package = pkg) == '') install.packages(pkg) | |
}) |
but too late compare to knitr option resolution.
So we do need to add it only for our documentation book and not the whole package in there
Line 93 in cedaac9
Config/Needs/book: remotes, webshot |
This is what is used for additional deps when building the book
bookdown/.github/workflows/Book.yaml
Lines 68 to 71 in cedaac9
- uses: r-lib/actions/setup-r-dependencies@v2 | |
with: | |
extra-packages: local::. | |
needs: book |
So can you move it ?
Thanks
No description provided.