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

Make sidebar()'s collapse icon configurable #830

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

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Oct 12, 2023

For example:

layout_sidebar(
  "Main content", 
  sidebar = sidebar(
    "Controls", 
    collapse_icon = bsicons::bs_icon("sliders")
  )
)
Screenshot 2023-10-12 at 4 14 31 PM

@cpsievert cpsievert force-pushed the sidebar-collapse-icon branch from 899330e to 5fc280e Compare October 12, 2023 21:13
@cpsievert cpsievert marked this pull request as draft October 12, 2023 21:13
@cpsievert cpsievert force-pushed the sidebar-collapse-icon branch 3 times, most recently from cf72028 to e0f6f13 Compare October 16, 2023 19:12
@cpsievert cpsievert force-pushed the sidebar-collapse-icon branch from e0f6f13 to 1e2e4dd Compare October 16, 2023 19:13
@cpsievert cpsievert requested a review from gadenbuie October 16, 2023 19:19
@cpsievert cpsievert marked this pull request as ready for review October 16, 2023 19:20
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Looks good! I realized we need to include aria-label on this button, which brings up the label we use for the toggle. I think we should go ahead and also make this configurable with a collapse_label argument.

R/sidebar.R Show resolved Hide resolved
R/sidebar.R Outdated Show resolved Hide resolved
@gadenbuie
Copy link
Member

I completely recognize that collapse_icon is a very simple solution to make the icon customizable. I'm realizing, though, that we're very likely to end up wanting to allow customizations in other aspects of the collapse toggle, like the label or the position, and we are facing a similar problem with the full screen toggle as well.

Given this, what if we were to encapsulate the collapse options into a single function, like sidebar_collapse_options()? This would give us some flexibility to change the collapse options in the future without changing the sidebar() API.

Such a function might look like this. Its arguments consult global options that can be set individually and it returns a named list so that we could have sidebar(collapse_options = sidebar_collapse_options()) as the default.

sidebar_collapse_options <- function(
  icon = getOption("bslib.sidebar.collapse_icon", NULL),
  label = getOption("bslib.sidebar.collapse_label", "Toggle Sidebar"),
  ...
) {
  opts <- list(icon = icon, label = label)
  names(opts) <- paste0("bslib.sidebar.collapse_", names(opts))
  class(opts) <- c("sidebar_collapse", "bslib_options")
  opts
}

sidebar_collapse_options()
#> $bslib.sidebar.collapse_icon
#> NULL
#> 
#> $bslib.sidebar.collapse_label
#> [1] "Toggle Sidebar"
#> 
#> attr(,"class")
#> [1] "sidebar_collapse" "bslib_options"

The list names are a little verbose, but that's to support passing sidebar_collapse_options() to options() to set the values globally:

options(
  sidebar_collapse_options(
    icon = bsicons::bs_icon("sliders"),
    label = "Toggle settings"
  )
)

getOption("bslib.sidebar.collapse_icon")
#> <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" class="bi bi-sliders " style="height:1em;width:1em;fill:currentColor;vertical-align:-0.125em;" aria-hidden="true" role="img" ><path fill-rule="evenodd" d="M11.5 2a1.5 1.5 0 1 0 0 3 1.5 1.5 0 0 0 0-3zM9.05 3a2.5 2.5 0 0 1 4.9 0H16v1h-2.05a2.5 2.5 0 0 1-4.9 0H0V3h9.05zM4.5 7a1.5 1.5 0 1 0 0 3 1.5 1.5 0 0 0 0-3zM2.05 8a2.5 2.5 0 0 1 4.9 0H16v1H6.95a2.5 2.5 0 0 1-4.9 0H0V8h2.05zm9.45 4a1.5 1.5 0 1 0 0 3 1.5 1.5 0 0 0 0-3zm-2.45 1a2.5 2.5 0 0 1 4.9 0H16v1h-2.05a2.5 2.5 0 0 1-4.9 0H0v-1h9.05z"></path></svg>

sidebar_collapse_options()
#> $bslib.sidebar.collapse_icon
#> <svg ... >
#> 
#> $bslib.sidebar.collapse_label
#> [1] "Toggle settings"
#> 
#> attr(,"class")
#> [1] "sidebar_collapse" "bslib_options"

This is also what I'm envisioning for full_screen_options().

@cpsievert cpsievert changed the title Make sidebar()'s collapse icon configurable; change default back to chevron-left Make sidebar()'s collapse icon configurable Oct 17, 2023
@cpsievert
Copy link
Collaborator Author

cpsievert commented Oct 19, 2023

For posterity, I'm in agreement that we need a way to customize the label (mainly for internationalization purposes), but I'm not convinced the cost of adding sidebar_collapse_options() is worth the benefit -- we already have a sidebar() for customizing the sidebar, and so it feels weird to adopt a different pattern for a specific subset of options.

That said, I do, in general, like the idea, and I'd probably be in favor of adopting it for card(fill_screen = full_screen_options()). However, if internationalization is the primary/sole motivating factor here, I may prefer to support something like options(bslib.sidebar_toggle_label = ..., bslib.card_full_screen_labels = ) for now.

And, since it seems logical to consider this in tandem with full_screen_options(), I'm thinking we'll stash this until the next release

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