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(page): Allow empty dots in page functions #1095

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Conversation

gadenbuie
Copy link
Member

This fixes allowing trailing empty arguments in all of the page functions. This was broken in several page functions because the dots must come after unnamed arguments in the calling functions.

Problem

Here's a minimal reprex that demonstrates the problematic pattern we were using:

page_one <- function(...) {
  bslib::page(..., "b",)
}

page_two <- function(...) {
  bslib::page("b", ...)
}

as.character(page_one("a", ))
#> Error in `dots_list()`:
#> ! Argument 2 can't be empty.
as.character(page_two("a", ))
#> [1] "<body>\n  b\n  a\n</body>"

In reality, we're not adding content but rather we're adding an unnamed dependency object (in the place of "b" above).

Before

library(bslib)
x <- "test"

page(x, ) |> print(browse = FALSE)
#> <body>test</body>

page_fillable(x, ) |> print(browse = FALSE)
#> Error in `dots_list()`:
#> ! Argument 5 can't be empty.

page_fixed(x, ) |> print(browse = FALSE)
#> Error in `dots_list()`:
#> ! Argument 3 can't be empty.

page_fluid(x, ) |> print(browse = FALSE)
#> Error in `dots_list()`:
#> ! Argument 3 can't be empty.

page_navbar(nav_panel(x), ) |> print(browse = FALSE)
#> <body class="bslib-page-fill bslib-gap-spacing bslib-flow-mobile bslib-page-navbar html-fill-container" style="padding:0px;gap:0px;">
#>   [...clip...]
#> </body>
page_sidebar(x, ) |> print(browse = FALSE)
#> <body class="bslib-page-fill bslib-gap-spacing bslib-flow-mobile bslib-page-sidebar html-fill-container" style="padding:0px;gap:0px;">
#>   [...clip...]
#> </body>

After

pkgload::load_all()
#> ℹ Loading bslib
x <- "test"

page(x, ) |> print(browse = FALSE)
#> <body>test</body>

page_fillable(x, ) |> print(browse = FALSE)
#> <body class="bslib-page-fill bslib-gap-spacing bslib-flow-mobile html-fill-container">test</body>

page_fixed(x, ) |> print(browse = FALSE)
#> <div class="container">test</div>

page_fluid(x, ) |> print(browse = FALSE)
#> <div class="container-fluid">test</div>

page_navbar(nav_panel(x), ) |> print(browse = FALSE)
#> <body class="bslib-page-fill bslib-gap-spacing bslib-flow-mobile html-fill-container bslib-page-navbar" style="padding:0px;gap:0px;">
#>   [...clip...]
#> </body>

page_sidebar(x, ) |> print(browse = FALSE)
#> <body class="bslib-page-fill bslib-gap-spacing bslib-flow-mobile html-fill-container bslib-page-sidebar" style="padding:0px;gap:0px;">
#>   [...clip...]
#> </body>

@gadenbuie gadenbuie requested a review from cpsievert July 28, 2024 17:31
Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

Thanks! Please also add unit tests for each page_() function

@cpsievert cpsievert changed the base branch from rc-v0.8.0 to main July 29, 2024 15:11
@cpsievert cpsievert changed the base branch from main to rc-v0.8.0 July 29, 2024 15:11
@cpsievert cpsievert merged commit 1410b2b into rc-v0.8.0 Jul 29, 2024
1 check passed
@cpsievert cpsievert deleted the fix-empty-dots branch July 29, 2024 15:11
cpsievert added a commit that referenced this pull request Jul 29, 2024
* Start v0.8.0 release candidate

* `yarn build` (GitHub Actions)

* ran revdepcheck on cloud. No known issues

* `yarn build` (GitHub Actions)

* re-run checks

* fix(page): Allow empty dots in page functions (#1095)

* fix empty trailing arguments in dots of page functions

* Add unit tests

---------

Co-authored-by: Carson <cpsievert1@gmail.com>

* Update snapshot tests

* Follow up to #1093: card_image() used to fill by default

* Fix R CMD check note about Rd links targets missing package anchors

* Missed a few

---------

Co-authored-by: cpsievert <cpsievert@users.noreply.github.com>
Co-authored-by: Barret Schloerke <barret@posit.co>
Co-authored-by: Garrick Aden-Buie <garrick@adenbuie.com>
Co-authored-by: gadenbuie <gadenbuie@users.noreply.github.com>
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.

2 participants