-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat!: Introduce navbar_options()
#1141
base: main
Are you sure you want to change the base?
Changes from 28 commits
01ff848
9ed36bb
0612d30
c726c6b
284d10f
8abc2b7
388720b
33732ae
f24c0c6
43e01c6
15c42d4
85b282a
a5b2868
5e1f0e2
b017dc5
7d473ee
fcd6d5c
3be0434
bd43c4d
a85ff8a
0fc9985
bf0f402
6f66bb7
76a9692
60b84ce
3f0d9bd
7028fba
bc87599
cf8d423
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -105,36 +105,220 @@ navset_hidden <- function(..., id = NULL, selected = NULL, | |||||
#' character vector, matching the `value` of [nav_panel()]s to be filled, may | ||||||
#' also be provided. Note that, if a `sidebar` is provided, `fillable` makes | ||||||
#' the main content portion fillable. | ||||||
#' @param bg a CSS color to use for the navbar's background color. | ||||||
#' @param inverse Either `TRUE` for a light text color or `FALSE` for a dark | ||||||
#' text color. If `"auto"` (the default), the best contrast to `bg` is chosen. | ||||||
#' @param navbar_options Options to control the appearance and behavior of the | ||||||
#' navbar. Use [navbar_options()] to create the list of options. | ||||||
#' @param position `r lifecycle::badge("deprecated")` Please use | ||||||
#' [`navbar_options = navbar_options(position=)`][navbar_options] instead. | ||||||
#' @param collapsible `r lifecycle::badge("deprecated")` Please use | ||||||
#' [`navbar_options = navbar_options(collapsible=)`][navbar_options] instead. | ||||||
#' @param bg `r lifecycle::badge("deprecated")` Please use | ||||||
#' [`navbar_options = navbar_options(bg=)`][navbar_options] instead. | ||||||
#' @param inverse `r lifecycle::badge("deprecated")` Please use | ||||||
#' [`navbar_options = navbar_options(inverse=)`][navbar_options] instead. | ||||||
#' | ||||||
#' @export | ||||||
#' @rdname navset | ||||||
navset_bar <- function(..., title = NULL, id = NULL, selected = NULL, | ||||||
sidebar = NULL, fillable = TRUE, | ||||||
gap = NULL, padding = NULL, | ||||||
# TODO: add sticky-top as well? | ||||||
position = c("static-top", "fixed-top", "fixed-bottom"), | ||||||
header = NULL, footer = NULL, | ||||||
bg = NULL, inverse = "auto", | ||||||
collapsible = TRUE, fluid = TRUE) { | ||||||
navset_bar <- function( | ||||||
..., | ||||||
title = NULL, | ||||||
id = NULL, | ||||||
selected = NULL, | ||||||
sidebar = NULL, | ||||||
fillable = TRUE, | ||||||
gap = NULL, | ||||||
padding = NULL, | ||||||
header = NULL, | ||||||
footer = NULL, | ||||||
fluid = TRUE, | ||||||
navbar_options = NULL, | ||||||
position = deprecated(), | ||||||
bg = deprecated(), | ||||||
inverse = deprecated(), | ||||||
collapsible = deprecated() | ||||||
) { | ||||||
padding <- validateCssPadding(padding) | ||||||
gap <- validateCssUnit(gap) | ||||||
|
||||||
navs_bar_( | ||||||
..., title = title, id = id, selected = selected, | ||||||
sidebar = sidebar, fillable = fillable, | ||||||
gap = gap, padding = padding, | ||||||
.navbar_options <- navbar_options_resolve_deprecated( | ||||||
options_user = navbar_options, | ||||||
position = position, | ||||||
header = header, footer = footer, | ||||||
bg = bg, inverse = inverse, | ||||||
collapsible = collapsible, fluid = fluid, | ||||||
bg = bg, | ||||||
inverse = inverse, | ||||||
collapsible = collapsible | ||||||
) | ||||||
|
||||||
navs_bar_( | ||||||
..., | ||||||
title = title, | ||||||
id = id, | ||||||
selected = selected, | ||||||
sidebar = sidebar, | ||||||
fillable = fillable, | ||||||
gap = gap, | ||||||
padding = padding, | ||||||
header = header, | ||||||
footer = footer, | ||||||
fluid = fluid, | ||||||
position = .navbar_options$position, | ||||||
bg = .navbar_options$bg, | ||||||
inverse = .navbar_options$inverse, | ||||||
collapsible = .navbar_options$collapsible, | ||||||
underline = .navbar_options$underline, | ||||||
# theme is only used to determine whether legacy style markup should be used | ||||||
# (and, at least at the moment, we don't need legacy markup for this exported function) | ||||||
theme = bs_theme() | ||||||
) | ||||||
} | ||||||
|
||||||
#' Create a set of navbar options | ||||||
#' | ||||||
#' A `navbar_options()` object captures options specific to the appearance and | ||||||
#' behavior of the navbar, independent from the content displayed on the page. | ||||||
#' This helper should be used to create the list of options expected by | ||||||
#' `navbar_options` in [page_navbar()] and [navset_bar()]. | ||||||
#' | ||||||
#' ## Changelog | ||||||
#' | ||||||
#' This function was introduced in \pkg{bslib} v0.9.0, replacing the `position`, | ||||||
#' `bg`, `inverse`, `collapsible` and `underline` arguments of [page_navbar()] | ||||||
#' and [navset_bar()]. Those arguments are deprecated with a warning and will be | ||||||
#' removed in a future version of \pkg{bslib}. | ||||||
#' | ||||||
#' @examples | ||||||
#' navbar_options(position = "static-top", bg = "#2e9f7d", underline = FALSE) | ||||||
#' | ||||||
#' @inheritParams shiny::navbarPage | ||||||
#' @param bg a CSS color to use for the navbar's background color. | ||||||
#' @param inverse Either `TRUE` for a light text color or `FALSE` for a dark | ||||||
#' text color. If `"auto"` (the default), the best contrast to `bg` is chosen. | ||||||
#' @param underline Whether or not to add underline styling to page or navbar | ||||||
#' links when active or focused. | ||||||
#' | ||||||
#' @returns Returns a list of navbar options. | ||||||
#' | ||||||
#' @export | ||||||
navbar_options <- function( | ||||||
position = c("static-top", "fixed-top", "fixed-bottom"), | ||||||
bg = NULL, | ||||||
inverse = "auto", | ||||||
collapsible = TRUE, | ||||||
underline = TRUE | ||||||
) { | ||||||
position2 <- rlang::arg_match(position) | ||||||
|
||||||
opts <- list( | ||||||
position = if (missing(position)) as_bslib_default(position[[1]]) else position2, | ||||||
bg = if (missing(bg)) as_bslib_default(bg) else bg, | ||||||
inverse = if (missing(inverse)) as_bslib_default(inverse) else inverse, | ||||||
collapsible = if (missing(collapsible)) as_bslib_default(collapsible) else collapsible, | ||||||
underline = if (missing(underline)) as_bslib_default(underline) else underline | ||||||
) | ||||||
|
||||||
structure(dropNulls(opts), class = c("bslib_navbar_options", "list")) | ||||||
} | ||||||
|
||||||
as_bslib_default <- function(x) { | ||||||
if (is.null(x)) return(x) | ||||||
attr(x, "bslib_default") <- TRUE | ||||||
attr(x, "waldo_opts") <- list(ignore_attr = TRUE) | ||||||
gadenbuie marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
x | ||||||
} | ||||||
|
||||||
is_bslib_default <- function(x) { | ||||||
isTRUE(attr(x, "bslib_default")) | ||||||
} | ||||||
|
||||||
navbar_options_resolve_deprecated <- function( | ||||||
options_user = NULL, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make it more clear this is a
Suggested change
|
||||||
position = deprecated(), | ||||||
bg = deprecated(), | ||||||
inverse = deprecated(), | ||||||
collapsible = deprecated(), | ||||||
underline = deprecated(), | ||||||
.fn_caller = "navset_bar", | ||||||
.warn_deprecated = TRUE | ||||||
) { | ||||||
options_old <- list( | ||||||
position = if (lifecycle::is_present(position)) position, | ||||||
bg = if (lifecycle::is_present(bg)) bg, | ||||||
inverse = if (lifecycle::is_present(inverse)) inverse, | ||||||
collapsible = if (lifecycle::is_present(collapsible)) collapsible, | ||||||
underline = if (lifecycle::is_present(underline)) underline | ||||||
) | ||||||
options_old <- dropNulls(options_old) | ||||||
|
||||||
args_deprecated <- names(options_old) | ||||||
|
||||||
if (.warn_deprecated && length(args_deprecated)) { | ||||||
lifecycle::deprecate_warn( | ||||||
"0.9.0", # 2024-11 | ||||||
I(sprintf( | ||||||
"The %s argument%s of `%s()` have been consolidated into a single `navbar_options` argument and ", | ||||||
paste(sprintf("`%s`", args_deprecated), collapse = ", "), | ||||||
if (length(args_deprecated) > 1) "s" else "", | ||||||
.fn_caller | ||||||
)) | ||||||
) | ||||||
} | ||||||
|
||||||
# Consolidate `navbar_options` (options_user) with direction options taking | ||||||
# the direct option if the user option is a default value, warning if | ||||||
# otherwise ignored. | ||||||
# TODO-deprecated Remove this and warning when direct options are hard-deprecated | ||||||
options_user <- options_user %||% list() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ignored <- c() | ||||||
for (opt in names(options_old)) { | ||||||
can_use_direct <- | ||||||
!opt %in% names(options_user) || | ||||||
is_bslib_default(options_user[[opt]]) | ||||||
|
||||||
if (can_use_direct) { | ||||||
options_user[[opt]] <- options_old[[opt]] | ||||||
} else if (!identical(c(options_old[[opt]]), c(options_user[[opt]]))) { | ||||||
ignored <- c(ignored, opt) | ||||||
} | ||||||
} | ||||||
|
||||||
if (length(ignored) > 0) { | ||||||
rlang::warn( | ||||||
c( | ||||||
sprintf( | ||||||
"`%s` %s provided twice: once directly and once in `navbar_options`.", | ||||||
paste(ignored, collapse = "`, `"), | ||||||
if (length(ignored) == 1) "was" else "were" | ||||||
), | ||||||
"The deprecated direct option(s) will be ignored and the values from `navbar_options` will be used." | ||||||
), | ||||||
call = rlang::caller_call() | ||||||
) | ||||||
} | ||||||
|
||||||
ret <- rlang::exec(navbar_options, !!!options_user) | ||||||
# strip bslib_default attributes now that we've resolved defaults | ||||||
lapply(ret, c) | ||||||
} | ||||||
|
||||||
#' @export | ||||||
print.bslib_navbar_options <- function(x, ...) { | ||||||
cat("<bslib_navbar_options>\n") | ||||||
|
||||||
if (length(x) == 0) { | ||||||
return(invisible(x)) | ||||||
} | ||||||
|
||||||
fields <- names(dropNulls(x)) | ||||||
opt_w <- max(nchar(fields)) | ||||||
for (opt in fields) { | ||||||
value <- x[[opt]] | ||||||
if (is_bslib_default(value)) { | ||||||
value <- sprintf("(%s)", value) | ||||||
} | ||||||
cat(sprintf("%*s", opt_w, opt), ": ", value, "\n", sep = "") | ||||||
} | ||||||
|
||||||
invisible(x) | ||||||
} | ||||||
|
||||||
# This internal version of navs_bar() exists so both it and page_navbar() | ||||||
# (and thus shiny::navbarPage()) can use it. And in the page_navbar() case, | ||||||
# we can use addition theme information as an indication of whether we need | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,8 +333,8 @@ maybe_page_sidebar <- function(x) { | |
#' | ||
#' @param fillable_mobile Whether or not `fillable` pages should fill the viewport's | ||
#' height on mobile devices (i.e., narrow windows). | ||
#' @param underline Whether or not to add underline styling to page links when | ||
#' active or focused. | ||
#' @param underline `r lifecycle::badge("deprecated")` Please use | ||
#' [`navbar_options = navbar_options(underline=)`][navbar_options] instead. | ||
#' @param window_title the browser window title. The default value, `NA`, means | ||
#' to use any character strings that appear in `title` (if none are found, the | ||
#' host URL of the page is displayed by default). | ||
|
@@ -397,31 +397,46 @@ page_navbar <- function( | |
fillable_mobile = FALSE, | ||
gap = NULL, | ||
padding = NULL, | ||
position = c("static-top", "fixed-top", "fixed-bottom"), | ||
header = NULL, | ||
footer = NULL, | ||
bg = NULL, | ||
inverse = "auto", | ||
underline = TRUE, | ||
collapsible = TRUE, | ||
navbar_options = NULL, | ||
fluid = TRUE, | ||
theme = bs_theme(), | ||
window_title = NA, | ||
lang = NULL | ||
lang = NULL, | ||
position = deprecated(), | ||
bg = deprecated(), | ||
inverse = deprecated(), | ||
underline = deprecated(), | ||
collapsible = deprecated() | ||
) { | ||
|
||
sidebar <- maybe_page_sidebar(sidebar) | ||
|
||
padding <- validateCssPadding(padding) | ||
gap <- validateCssUnit(gap) | ||
|
||
# Change behavior when called by Shiny | ||
# TODO: Coordinate with next bslib version bump in Shiny to use the new interface | ||
was_called_by_shiny <- | ||
isNamespaceLoaded("shiny") && | ||
identical(rlang::caller_fn(), shiny::navbarPage) | ||
|
||
.navbar_options <- navbar_options_resolve_deprecated( | ||
options_user = navbar_options, | ||
position = position, | ||
bg = bg, | ||
inverse = inverse, | ||
collapsible = collapsible, | ||
underline = underline, | ||
.fn_caller = "page_navbar", | ||
.warn_deprecated = !was_called_by_shiny | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would really really like to decouple What I want out of that change is to have a clear line around what parts of the navbar API we shouldn't touch without thinking carefully about backwards compatibility or API changes and which parts are owned by bslib. Currently it feels too entangled and over time entropy wins leading to more entanglement. |
||
) | ||
|
||
# Default to fillable = F when this is called from shiny::navbarPage() | ||
# TODO: update shiny::navbarPage() to set fillable = FALSE and get rid of this hack | ||
if (missing(fillable)) { | ||
isNavbarPage <- isNamespaceLoaded("shiny") && identical(rlang::caller_fn(), shiny::navbarPage) | ||
if (isNavbarPage) { | ||
fillable <- FALSE | ||
} | ||
if (missing(fillable) && was_called_by_shiny) { | ||
fillable <- FALSE | ||
} | ||
|
||
# If a sidebar is provided, we want the layout_sidebar(fill = TRUE) component | ||
|
@@ -439,13 +454,23 @@ page_navbar <- function( | |
class = "bslib-page-navbar", | ||
class = if (!is.null(sidebar)) "has-page-sidebar", | ||
navs_bar_( | ||
..., title = title, id = id, selected = selected, | ||
sidebar = sidebar, fillable = fillable, | ||
gap = gap, padding = padding, | ||
position = match.arg(position), header = header, | ||
footer = footer, bg = bg, inverse = inverse, | ||
underline = underline, collapsible = collapsible, | ||
fluid = fluid, theme = theme | ||
..., | ||
title = title, | ||
id = id, | ||
selected = selected, | ||
sidebar = sidebar, | ||
fillable = fillable, | ||
gap = gap, | ||
padding = padding, | ||
header = header, | ||
footer = footer, | ||
position = .navbar_options$position, | ||
bg = .navbar_options$bg, | ||
inverse = .navbar_options$inverse, | ||
underline = .navbar_options$underline, | ||
collapsible = .navbar_options$collapsible, | ||
fluid = fluid, | ||
theme = theme | ||
) | ||
) | ||
} | ||
|
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.
Should we include
...
here to make future expansion easier? Maybe even making all arguments required?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.
I made this change. We'll need the cover from
...
and because there's no clear ordering I'd rather not encourage the positional argument usage that would make future expansion difficult.