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(navbar): Improve navbar colors in dark mode #1135

Closed
wants to merge 17 commits into from
Closed
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions R/navs-legacy.R
Original file line number Diff line number Diff line change
@@ -149,6 +149,13 @@ navs_bar_ <- function(..., title = NULL, id = NULL, selected = NULL,
collapsible = TRUE, fluid = TRUE,
theme = NULL) {

navbar_color_mode <- switch(
inverse,
gadenbuie marked this conversation as resolved.
Show resolved Hide resolved
"TRUE" = "dark",
"FALSE" = "light",
inverse
)

if (identical(inverse, "auto")) {
inverse <- TRUE
if (identical(theme_preset_info(theme)$name, "shiny")) {
@@ -171,12 +178,12 @@ navs_bar_ <- function(..., title = NULL, id = NULL, selected = NULL,
theme = theme
)

if (!is.null(bg)) {
# navbarPage_() returns a tagList() of the nav and content
navbar[[1]] <- tagAppendAttributes(
navbar[[1]], style = css(background_color = paste(bg, "!important"))
)
}
# navbarPage_() returns a tagList() of the nav and content
navbar[[1]] <- tagAppendAttributes(
navbar[[1]],
style = if (!is.null(bg)) css(background_color = paste(bg, "!important")),
"data-bs-theme" = navbar_color_mode
cpsievert marked this conversation as resolved.
Show resolved Hide resolved
)

as_fragment(navbar, page = page)
}
@@ -211,7 +218,7 @@ navbarPage_ <- function(title,
position <- match.arg(position)
if (!is.null(position))
navbarClass <- paste0(navbarClass, " navbar-", position)
if (inverse)
if (isTRUE(inverse))
cpsievert marked this conversation as resolved.
Show resolved Hide resolved
navbarClass <- paste(navbarClass, "navbar-inverse")

if (!is.null(id))
9 changes: 0 additions & 9 deletions inst/bs3compat/_navbar_compat.scss
Original file line number Diff line number Diff line change
@@ -101,15 +101,6 @@ ul.nav.navbar-nav {
}
}

$enable-dark-mode: false !default;
@if $enable-dark-mode {
@include color-mode(dark) {
.navbar.navbar-default {
background-color: var(--bslib-navbar-default-bg, var(--#{$prefix}dark)) !important;
}
}
}
cpsievert marked this conversation as resolved.
Show resolved Hide resolved

// Implement bs3 navbar toggler; used in Rmd websites, i.e.
// https://github.com/rstudio/rmarkdown-website/blob/453e1802b32b5baf1c8a67f80947adcc53e49b7f/_navbar.html
.navbar-toggle {
2 changes: 1 addition & 1 deletion inst/css-precompiled/5/bootstrap.min.css

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion inst/lib/bs5/scss/_navbar.scss
Original file line number Diff line number Diff line change
@@ -307,11 +307,17 @@
--#{$prefix}navbar-toggler-border-color: #{$navbar-dark-toggler-border-color};
--#{$prefix}navbar-toggler-icon-bg: #{escape-svg($navbar-dark-toggler-icon-bg)};
// scss-docs-end navbar-dark-css-vars

// patched: toggler icon should follow closest navbar color mode over global mode
.navbar-toggler-icon {
--#{$prefix}navbar-toggler-icon-bg: #{escape-svg($navbar-dark-toggler-icon-bg)};
}
}

@if $enable-dark-mode {
@include color-mode(dark) {
.navbar-toggler-icon {
// patched: toggler follows global theme unless in a light region
.navbar:not([data-bs-theme="light"]) .navbar-toggler-icon {
--#{$prefix}navbar-toggler-icon-bg: #{escape-svg($navbar-dark-toggler-icon-bg)};
}
Comment on lines +320 to 323
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this gonna work sensibly for scenarios where we don't have control over the navbar markup (e.g., pkgdown, rmarkdown, flexdashboard, etc.)?

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 can look into this and will add anything that needs to be addressed to posit-dev/brand-yml#52. Bringing pkgdown and bslib back into alignment is part of that plan.

Copy link
Member Author

Choose a reason for hiding this comment

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

This whole PR does interact with rmarkdown websites and the approach I'd like to take is to make .navbar-default and .navbar-inverse be no-op classes for BS 5+. We could also take them out of the markup when we know that we're in BS 5+. That's going to require another pass at this PR, though.

}
23 changes: 23 additions & 0 deletions tools/patches/034-bs5-navbar-toggler-icon-dark-mode.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
diff --git a/inst/lib/bs5/scss/_navbar.scss b/inst/lib/bs5/scss/_navbar.scss
index 988bbe09..dcd3c28a 100644
--- a/inst/lib/bs5/scss/_navbar.scss
+++ b/inst/lib/bs5/scss/_navbar.scss
@@ -307,11 +307,17 @@
--#{$prefix}navbar-toggler-border-color: #{$navbar-dark-toggler-border-color};
--#{$prefix}navbar-toggler-icon-bg: #{escape-svg($navbar-dark-toggler-icon-bg)};
// scss-docs-end navbar-dark-css-vars
+
+ // patched: toggler icon should follow closest navbar color mode over global mode
+ .navbar-toggler-icon {
+ --#{$prefix}navbar-toggler-icon-bg: #{escape-svg($navbar-dark-toggler-icon-bg)};
+ }
}

@if $enable-dark-mode {
@include color-mode(dark) {
- .navbar-toggler-icon {
+ // patched: toggler follows global theme unless in a light region
+ .navbar:not([data-bs-theme="light"]) .navbar-toggler-icon {
--#{$prefix}navbar-toggler-icon-bg: #{escape-svg($navbar-dark-toggler-icon-bg)};
}
}