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

feat!: Introduce navbar_options() #1141

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

feat!: Introduce navbar_options() #1141

wants to merge 29 commits into from

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Nov 26, 2024

This PR consolidates the position, bg, collapse, inverse and underline arguments of navset_bar() and page_navbar() into a navbar_options() helper and replaces their use in the parent functions with a single navbar_options argument.

Before

page_navbar(
  title = "My App",
  bg = "#0062cc",
  underline = TRUE,
  nav_panel() # ...
)

After

page_navbar(
  title = "My App",
  navbar_options = navbar_options(
    bg = "#0062cc",
    underline = TRUE
  ),
  nav_panel() # ...
)

Using the direct arguments results in a lifecycle deprecation warning

page_navbar(
  title = "My App",
  bg = "#0062cc"
)
#> Warning message:
#> The `bg` argument of `page_navbar()` is deprecated as of bslib 0.9.0.
#> ℹ Navbar options have been consolidated into a single `options` argument. Use the `bg`
#>   argument of `navbar_options()` instead.
#> ℹ The deprecated feature was likely used in the bslib package.
#>   Please report the issue at <https://github.com/rstudio/bslib/issues>.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was generated. 

and another warning is emitted if the same argument is used in both places

page_navbar(
  title = "My App",
  bg = "#0062cc",
  navbar_options = navbar_options(bg = "#cc0099")
)
#> Warning message:
#> In page_navbar(title = "My App", bg = "#0062cc", navbar_options = navbar_options(bg = "#cc0099")) :
#>   `bg` was provided twice: once directly and once in `navbar_options`.
#> • The deprecated direct option(s) will be ignored and the values from `navbar_options` will be used.

Background

The reason for this PR is two-fold:

  1. The arguments of page_navbar() and navset_bar() are applied in several different contexts, but there are clear groupings. This change makes it clearer which arguments apply to the navbar appearance and style by separating them from the remaining arguments.

  2. This change makes it easier to introduce new navbar options to page_navbar() and navset_bar(). Bootstrap 5 has moved away from the default/inverse terminology and has improved the terminology around the navbar colors. (Is inverse = TRUE for light on dark background or dark text on light background? I can never remember.)

    Bootstrap 5 now uses data-bs-theme="dark" and the color mode terminology, where "light" is like a light color mode, i.e. dark text on a light background. It'd be nice to make the navbar functions consistent with this new language in a backwards-compatible way.

    However, if we were to introduce mode or type as a top-level argument, it'd be far too confusing with the other arguments. If we introduce navbar_mode or navbar_type, it's equally confusing -- why is only mode or type prefixed with navbar_? Introducing a navbar_options() helper gives us the space to have better named arguments. It also creates space to have Bootstrap version-aware options and defaults.

@gadenbuie gadenbuie marked this pull request as ready for review November 26, 2024 19:33
collapsible = collapsible,
underline = underline,
.fn_caller = "page_navbar",
.warn_deprecated = !was_called_by_shiny
Copy link
Member Author

Choose a reason for hiding this comment

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

I would really really like to decouple shiny::navbarPage() from bslib::page_navbar(). I'm thinking it'd be an update to add page_navbar_legacy() that we'd export but mark as internal so it doesn't show up in docs and indices.

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.

R/navs-legacy.R Outdated Show resolved Hide resolved
R/navs-legacy.R Outdated Show resolved Hide resolved
@cpsievert
Copy link
Collaborator

cpsievert commented Nov 26, 2024

This change makes it easier to introduce new navbar options to page_navbar() and navset_bar().

Do you have any immediate/near-term plans to do this?

@gadenbuie
Copy link
Member Author

This change makes it easier to introduce new navbar options to page_navbar() and navset_bar().

Do you have any immediate/near-term plans to do this?

Yeah, I'm planning to immediately use this to introduce navbar_options(type = "dark") as a replacement for inverse. See posit-dev/brand-yml#52 for (long and detailed) background.

@cpsievert
Copy link
Collaborator

Yeah, I'm planning to immediately use this to introduce navbar_options(type = "dark")

Sounds good, thanks. FWIW, I do want to be conservative about adding more to the bslib:: namespace, so I wasn't sure about this at first, but I can see now why this is worth it.

R/navs-legacy.R Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/navs-legacy.R Outdated Show resolved Hide resolved
@cpsievert
Copy link
Collaborator

Here is the idea I had earlier, but had a hard time articulating. It does drop the extra warning about navbar_options() taking precedence, but I feel like that's overkill anyway given you'll also get a deprecation warning in that case:

simplify.patch

@gadenbuie
Copy link
Member Author

Here is the idea I had earlier, but had a hard time articulating. It does drop the extra warning about navbar_options() taking precedence, but I feel like that's overkill anyway given you'll also get a deprecation warning in that case:

Thanks for the suggestion. I also tried something similar this morning, but I think it's important that navbar_options() is equivalent to a standard list. With the current implementation, navbar_options(bg = "red") and list(bg = "red") are equivalent. That feels like an important constraint to me.

I also think it's important to consider that the code is currently a little complicated to give the best transition experience. In a year or so we could delete most of it and hard-deprecate the direct arguments (once we deal with shiny::navbarPage()).

R/navs-legacy.R Outdated
Comment on lines 200 to 206
navbar_options <- function(
position = c("static-top", "fixed-top", "fixed-bottom"),
bg = NULL,
inverse = "auto",
collapsible = TRUE,
underline = TRUE
) {
Copy link
Member Author

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?

Copy link
Member Author

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.

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