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 <[email protected]>
… 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.
There was a problem hiding this comment.
Note: type is now theme
There was a problem hiding this comment.
Also worth noting that "auto" in bslib becomes data-bs-theme="auto", which is really just a stand-in for neither data-bs-theme="light" nor data-bs-theme="dark". We don't intend to specifically target [data-bs-theme="auto"] at any point.
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
R/navs-legacy.R
Outdated
| collapsible = TRUE, | ||
| underline = TRUE | ||
| ) { | ||
| # Track user-provided arguments for print method and deprecation warnings | ||
| is_default <- list( | ||
| position = missing(position), | ||
| bg = missing(bg), | ||
| inverse = missing(inverse), | ||
| type = missing(type), |
There was a problem hiding this comment.
type was always a "least bad" choice, but after coming back to this I really don't like it. I'm going to replace type with theme, which seems to be a reasonable fit. theme matches how we end up using it i.e. theme = "dark" → data-bs-theme="dark".
I also considered style (overlaps with HTML attribute) and mode (Bootstrap calls it "color mode", but mode isn't very clear or specific).
#1145) * fix(navbar): Don't flip navbar bg in dark mode This rule only changes the background color of the navbar, which is not enough to fully restyle the navbar in dark mode. It might work in the default cases, but it certainly does not work if users have set $navbar-dark-bg or $navbar-light-bg to appropriate values. Note that Bootstrap expects the navbar colors to be static (i.e. the same in light and dark mode), so if we want to support different navbar colors we'll need to implement something better. * fix(bootstrap): Navbar toggler icon should follow navbar color mode Bootstrap has the navbar toggler icon follow the global color mode, which makes the icon invisible when placed on a light background in dark mode. Outside of this rule, Bootstrap expects the navbar color to be consistent in light and dark mode. * fix(bootstrap): Keep global color mode follow * fix: Fix `_navbar.scss` patch * feat: Only use `.navbar-default` and `.navbar-inverse` classes in BS <5 * fix(navbar_compat): navbar light background * feat: account for navbar in light mode region * feat: use `--bslib-navbar-{light,dark}-bg` variables * feat(shiny-preset): Use border-bottom for default navbar not in a dashboard page * refactor(shiny-preset): Use Sass vars to set navbar light/dark bg * fix(bs5): Selector on light-region navbar-toggler-icon * fix(navbar): Dark/light adjustments need to have the lowest specificity possible Otherwise they get in the way of local classes, e.g. `bg-blue` or `bg-primary`, which are the primary mechanism for setting navbar color in BS5 https://getbootstrap.com/docs/5.3/components/navbar/#color-schemes * fix(navbar): Use `:where()` to reduce navbar dark/light styles to minimal specificity * fix(navbar): Use `:where()` when setting navbar bg to ensure that utility classes will win * feat!(navbars): Don't use BS3 compat for navbars in BS5+ * chore: whitespace fixes
| @@ -6,6 +6,8 @@ | |||
|
|
|||
| Related to the above change, `navset_bar()` now defaults to using `underline = TRUE` so that both `page_navbar()` and `navset_bar()` use the same set of default `navbar_options()`. | |||
|
|
|||
| In `navbar_options()`, `inverse` is replaced by `theme`, which takes values `"light"` (dark text on a **light** background), `"dark"` (light text on a **dark** background), or `"auto"` (follow page settings). (#1146) | |||
There was a problem hiding this comment.
@gadenbuie couldn't we just change inverse to theme in the 1st bullet point (since navbar_options() is a new addition in this release) instead of adding another bullet?
There was a problem hiding this comment.
I didn't change it because that sentence is talking about the direct arguments, which is still inverse. I could move this sentence, but I don't mind calling out the change on its own line.
This PR was extracted from #1145 for easier review. The goal of this PR is:
Replace
inversewithtypethemeinnavbar_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.