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

fix: use BS version from template package again #2441

Closed
wants to merge 1 commit into from
Closed

fix: use BS version from template package again #2441

wants to merge 1 commit into from

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Apr 15, 2024

Fix #2440 cc @gadenbuie

@Bisaloo could you please confirm it also works for you?

This is not an elegant fix because of getting the BS version twice. I can also imagine it'd make sense to ask maintainers of template packages to ask their users to set the BS version? But it means more work if updating the BS version in the template package so I would not appreciate this outcome as a template package maintainer. 😹

@maelle
Copy link
Collaborator Author

maelle commented Apr 15, 2024

Now I also realize that if the pkgdown package template is used by a central docs building thing, it's not too hard to set the BS version in an override. 😅

@maelle
Copy link
Collaborator Author

maelle commented Apr 15, 2024

This section https://pkgdown.r-lib.org/articles/customise.html#template-packages states that one can use the configuration file of the template package to set the Bootstrap version.

@@ -32,6 +32,9 @@ as_pkgdown <- function(pkg = ".", override = list()) {
)
meta <- modify_list(template_config, meta)

# BS version can also be set by template package itself
bs_version <- get_bootstrap_version(list(meta = meta))
Copy link
Member

Choose a reason for hiding this comment

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

Given that this function is the only place that get_bootstrap_version() is used, I think it might be slightly clearer to do something like:

# Update bs_version if set in template
bs_version <- bs_version %||% template_config$bootstrap 

@gadenbuie
Copy link
Contributor

I think I see the problem I introduced in #2395. It's somewhat nuanced, so I'm working on adding tests. Thanks for starting this PR @maelle and for tagging me! I'll submit a separate PR for an easier review.

@maelle
Copy link
Collaborator Author

maelle commented Apr 16, 2024

Thanks both!!

SebKrantz pushed a commit to SebKrantz/pkgdown that referenced this pull request Jun 1, 2024
@hadley hadley deleted the debug branch July 8, 2024 07:23
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.

pkgdown now demands to set BS version in config when using a template package
4 participants