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

Various improvements to tab panels #3315

Merged
merged 31 commits into from
Mar 22, 2021
Merged

Various improvements to tab panels #3315

merged 31 commits into from
Mar 22, 2021

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Mar 4, 2021

This PR closes numerous issues with tabsetPanel()/navlistPanel()/navbarPage(). See the respective issue links for examples:

  1. !!! may now be used to splice tabPanel()s into them (closes tabsetPanel() doesn't work with list of panels #2927)
  2. The tabsetPanel() and navlistPanel() functions gain header/footer arguments, akin to the already existing header/footer arguments in navbarPage(), making it easier to include content that should appear on every tab (closes Make it easier to include header/footer content in tabsetPanel()/navlistPanel()  #3322).
  3. NULL values are now dropped instead of producing an empty nav item (closes tabsetPanel doesn't ignore NULLs #1928).
  4. More informative error when non-tabPanel()/shiny.tag objects are supplied to ... (closes tabsetPanel() et al give confusing error messages when they receive incorrect input #3313, closes tabSetpanel first argument #1823).
  5. New informative warning when shiny.tag object(s) are supplied to .... In this case we will continue to create an "empty" nav item and include the content on every tab, but the warning will mention the (new) header/footer args, which is likely what the user wants (closes Tab panel behavior when shiny tags are supplied  #3321).
  6. Bootstrap 4+ HTML/CSS markup is now produced when used with theme = bslib::bs_theme() (closes Tab panel functions don't produce Bootstrap 4 compatible markup #3320). At least for now, you'll still want {bslibs}'s BS3 compatibility layer to provide sensible styles in navlistPanel() and navbarPage(), but perhaps in follow up PRs we can explore completely removing the compatibility layer dependency for those as well. The main downside to this change in behavior is that tabsetPanel() now returns tagFunction()s, meaning that user code which makes assumptions about the return value may break, but hopefully by providing more intelligent tag modification functions (see, for example, Add tagFunction() awareness to tag modifying functions htmltools#202), this won't be much of an issue.

Testing notes

Install remotes::install_github("rstudio/shiny") then verify there is no regression in the example app above as well as the one below (in the app below, when you click add 'dynamic' tab, a tab should be inserted between Static1 and Static2 in the dropdown)

library(shiny)
library(bslib)

ui <- fluidPage(
  theme = bs_theme(),
  sidebarLayout(
    sidebarPanel(
      actionButton("add", "Add 'Dynamic' tab"),
      actionButton("remove", "Remove 'Foo' tab")
    ),
    mainPanel(
      tabsetPanel(
        id = "tabs",
        tabPanel("Hello", "This is the hello tab"),
        tabPanel("Foo", "This is the foo tab"),
        navbarMenu(
          "Static", 
          tabPanel("Static 1", "Static 1", value = "s1"),
          tabPanel("Static 2", "Static 2", value = "s2")
        )
      )
    )
  )
)
server <- function(input, output, session) {
  observeEvent(input$add, {
    insertTab(
      inputId = "tabs",
      tabPanel("Dynamic", "Dynamic"),
      target = "s2"
    )
  })
  observeEvent(input$remove, {
    removeTab(inputId = "tabs", target = "Foo")
  })
}

shinyApp(ui, server)

R/bootstrap.R Outdated
Comment on lines 991 to 995
# Replace <li class="nav-item"><a class="nav-link"></a></li>
# with <a class="dropdown-item"></a>
navItem <- x$children[[1]]
navItem$attribs$class <- "dropdown-item"
navItem
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main unfortunate thing about all of this is that BS4 doesn't support <li><a></a></li> inside of dropdowns, which apparently they brought back in BS5

https://getbootstrap.com/docs/4.6/components/navs/#tabs-with-dropdowns
https://getbootstrap.com/docs/5.0/components/navs-tabs/#tabs-with-dropdowns

Might be worth asking for a backport into BS4, but I doubt we'll see it

@cpsievert cpsievert requested a review from wch March 4, 2021 15:25
R/bootstrap.R Show resolved Hide resolved
R/bootstrap.R Show resolved Hide resolved
@cpsievert cpsievert changed the title 'Native' Bootstrap 4 tabset panel support Various improvements to tab panels Mar 6, 2021
R/bootstrap.R Outdated Show resolved Hide resolved
@cpsievert cpsievert added this to the 1.7 milestone Mar 12, 2021
R/shiny-package.R Outdated Show resolved Hide resolved
R/html-deps.R Outdated Show resolved Hide resolved
}

# TODO: something like this should exist in htmltools
tagHasClass <- function(x, class) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The complexity of this code inspired this issue:
rstudio/htmltools#205

tests/testthat/test-tabPanel.R Outdated Show resolved Hide resolved
@cpsievert cpsievert merged commit 32d0e14 into master Mar 22, 2021
@cpsievert cpsievert deleted the bs4-tab-panels branch March 22, 2021 17:38
shalutiwari added a commit to rstudio/shinycoreci-apps that referenced this pull request Apr 13, 2021
cpsievert added a commit that referenced this pull request Apr 19, 2021
cpsievert added a commit that referenced this pull request May 5, 2021
* Follow up to #3315: reduce complexity and 'black-boxed' nature of tab panel logic

* asTags(selected = FALSE) is now root()

* tagAddRenderHook

* Add bslib to remotes

* Document (GitHub Actions)

* root() was recently changed to allTags()

* code review

* tagQuery() doesn't necessarily preserve order of attributes

* place href attribute before data attributes

* add nav-item/nav-link to BS4+ dropdowns

* Make sure .nav-item is removed in .dropdown-menu

Co-authored-by: cpsievert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment