feat(navbar_options): Match Bootstrap 5 semantics #1146
Conversation
Lets `inverse` be a character value that is used directly for `data-bs-theme`. Turns out this is a backwards compatible change that opens the door to the future addition of another argument.
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
… container Fixes #940
…tribs` for consistency
R/navs-legacy.R
Outdated
| inverse <- TRUE | ||
| if (identical(theme_preset_info(theme)$name, "shiny")) { | ||
| inverse <- FALSE | ||
| if (is.null(theme) || theme_version(theme) < 5) { | ||
| inverse <- TRUE |
There was a problem hiding this comment.
I think I'm OK with this, but would you mind writing down the thought process behind it?
Also, this will change navbar appearance for Bootswatch themes (and vanilla Bootstrap), so assuming we're OK with that, let's make sure to call it out as a breaking change in the NEWS
There was a problem hiding this comment.
Sorry, I think the code is easier to review with the smaller PRs, but the context is a bit spread out. The motivation is outlined in the first part of the description in #1145.
In short, in Bootstrap 5 inverse = type = "auto" now means "match the current light/dark mode". Before this, it "auto" meant "pick for me".
We could get into setting class and type attributes here to get navbar colors that match the current defaults. The continuity would be nice, but it'd add work and complexity that I'm not sure is worth the effort.
R/navs-legacy.R
Outdated
| 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() | ||
| ) | ||
|
|
||
| navbar_options_apply_attribs(navbar, .navbar_options) |
There was a problem hiding this comment.
I think navbar_options_apply_attribs() could be easier to reason about if you assume that it gets passed a tag object and a list of attributes.
| navbar_options_apply_attribs(navbar, .navbar_options) | |
| navbar[[1]] <- navbar_options_apply_attribs(navbar[[1]], .navbar_options$attribs) | |
| navbar |
There was a problem hiding this comment.
Thanks, this hits on something I was noticing also, which is that navs_bar_() is not-quite legacy code, since it involves handling BS 5 styles. In this PR and others I was trying to hold a line where navs_bar_() was touched as little as possible for legacy reasons.
But this, and a few other changes, would be much better handled inside navs_bar_() than from the outside.
There was a problem hiding this comment.
Ok, yea, I see where you're coming from. Feel free to resolve this one
There was a problem hiding this comment.
I moved all of the logic of navbar_options_apply_attribs() into navs_bar_(), which cleans up a lot of this logic and removed the need for a few other comments that I've resolved.
There was a problem hiding this comment.
Nice, I like it, thanks!
man/figures/page-navbar.png
Outdated
There was a problem hiding this comment.
Will this difference go away with #1145?
There was a problem hiding this comment.
No probably not. It comes from the navbar having data-bs-theme="dark" ... which is how Bootstrap recommends doing this. I'll look into it more to see if there's something we're missing.
There was a problem hiding this comment.
Here's a stackblitz example based on the Bootstrap docs examples.
The way around that is to put make sure the dropdown menu resets to light.
<ul class="dropdown-menu" data-bs-theme="light">
Allows us to avoid needing to do surgery on the navbar later
This PR was extracted from #1145 for easier review. The goal of this PR is:
Replace
inversewithtypeinnavbar_options(). This replaces an overloaded binary option with an enum that currently accepts"auto"(default),"light"or"dark". There is still room for confusion around the meaning of light and dark, but it's easy to define and consistently used throughout bslib, Bootstrap, brand.yml, Quarto, etc. (light means light background, dark foreground, dark means dark background, light foreground).Include
data-bs-themein the navbar markup in a backwards-compatible way.Allow
navbar_options(...)to include attributes that are passed to the nav container, making it possible to setclass = "bg-primary"or other attributes, e.g.style. Users could also setdata-bs-themehere directly, since that syntax appears in Bootstrap and Bootswatch docs.Note that this PR does not change navbar appearance, so the scope of this PR is from R code to HTML markup.
FYI: In a follow-up PR I plan to move
navbar_options()and related helpers fromR/navs-legacy.RtoR/navbar_options.R.