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

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

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Nov 19, 2024

This rule removed in this PR 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, leading to inaccessible values (e.g. white text on a light background).

This PR removes the changing of the navbar background color in dark mode. This is inline with upstream expectations: Bootstrap expects the navbar colors to be static (i.e. the same in light and dark mode).

If we want to support different navbar colors we'll need to implement something that compliments the current setup.

This PR also sets data-bs-theme="dark" on the navbar <nav> element when inverse = TRUE and fixes the navbar toggle icon in dark mode.

Important

Re-test after merging #1134

Before After Description
image image Dark mode with inverse = FALSE, black on light blue is expected but white toggle button icon is not.
image image Dark mode with inverse = TRUE, white text on dark background is expected in both light/dark, before shows that light navbar bg was used in dark mode.
Example app
library(shiny)
pkgload::load_all()

ui <- page_navbar(
  title = "My Simple App",
  theme = bs_theme(
    preset = "default",
    navbar_bg = "#732400",
    navbar_light_bg = "#b2d8ff",
    navbar_dark_bg = "#00165d",
    cache_buster = format(as.integer(Sys.time()))
  ),
  inverse = TRUE,
  sidebar = sidebar(
    title = "Settings",
    "Just stuff here",
    input_dark_mode(),
  ),
  nav_panel("Home",
    h1("Welcome to the Home Page"),
    p("This is some boilerplate text for the home page."),
    card(
      card_header("Just a regular card"),
      layout_sidebar(
        sidebar = sidebar(
          "Some stuff here too",
          position = "right"
        ),
        "And card content with a sidebar!"
      )
    )
  )
)

server <- function(input, output, session) {

}

shinyApp(ui, server)

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.
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.
@gadenbuie gadenbuie marked this pull request as draft November 19, 2024 15:18
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.
@gadenbuie gadenbuie marked this pull request as ready for review November 19, 2024 15:31
@gadenbuie gadenbuie changed the title fix(navbar): Don't flip navbar bg in dark mode fix(navbar): Improve navbar colors in dark mode Nov 19, 2024
@gadenbuie gadenbuie changed the base branch from fix/navbar-sass-vars to main November 19, 2024 16:31
Comment on lines +319 to 322
// 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)};
}
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.

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

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

LGTM once the remaining suggestion is addressed. Lets also remember to get the equivalent py-shiny changes in soon after merging

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