Dynamic tabs #1794

bborgesr · 2017-07-14T17:31:32Z

This introduces 6 new functions to Shiny:

insertTab (signature: insertTab(inputId, tab, target, position = c("before", "after")))
prependTab (signature: prependTab(inputId, tab, menuName = NULL))
appendTab (signature: appendTab(inputId, tab, menuName = NULL))
removeTab (signature: removeTab(inputId, target))
hideTab (signature: hideTab(inputId, target))
showTab (signature: showTab(inputId, target))

An app for dynamic testing and exploring (includes all possibilities): https://beta.rstudioconnect.com/content/2917/

Here are the three UI tabset-like containers that these functions apply to (i.e. an instance of one of these things must be provided to the desired function through the inputId arg):

navbarPage (including navbarMenu -- see more below)
tabsetPanel (both types)
navlistPanel

There is also the case of navbarMenu, which is a dropdown menu that a navbarPage can have (instead of the standard tabPanelitems, though those are fine as well). A navbarMenu can be inserted/removed/hidden/shown, just like a tabPanel (for the targetargument, supply the menuName of the navbarMenu, instead of the value of the tabPanel). You can also add more tabPanels or text (dividers and header) inside a navbarMenu.

For example, consider we have a navbarPage (with id "navbarPage") that has a navbarMenu (with menuName "menu"), which itself includes another two tabs ("Foo" and "Bar"). Then, using insertTab(inputId = "navbarPage", target = "Foo", tab = ...) will insert the tab before "Foo" (notice how you don't need to supply the name of the navbarMenu, since target = "Foo" already gives us the necessary info).

If you instead want to prepend/append a tab to the beginning/end of a navbarMenu, you do need to supply the menuName:

prependTab(inputId = "navbarPage", menuName = "menu", tab = ...)
appendTab(inputId = "navbarPage", menuName = "menu", tab = ...)

wch · 2017-07-14T18:38:13Z

R/insert-ui.R

+}
+
+
+#' Dynamically remove a tabPanel


I think it would make sense to put the help for all of the new functions together into a single help file, using @rdname.

I struggle a bit with this. I think it definitely makes sense to link the functions around (maybe even have the same description at the top, with links to the docs of four all functions right there). So, why not put it all in a single help file, like you suggest? My thinking was that I, as an user, would much prefer to have one help file per function, since that allows me to have a usage example for that function only. Since these are related, but different, functions, seeing a usage example for each one could be beneficial (or at least that's my theory). I'm really not sure though, so I'd love to know your and @jcheng5's opinion about this, since I imagine it's common "problem" when writing documentation for family of related, but different, functions...

wch · 2017-07-14T18:44:24Z

R/insert-ui.R

+
+  iconClass <- tab$attribs$`data-icon-class`
+  icon <- if (!is.null(iconClass)) {
+    # for font-awesome we specify fixed-width


Why is this needed for font awesome (and not for glyphicons)?

This was copied from here: (R/bootstrap.R#L818-L823)[https://github.com/rstudio/shiny/blob/master/R/bootstrap.R#L818-L823]. I admit i didn't really look into it, but I can if neither you or @jcheng5 remember this. Let me know!

wch · 2017-07-14T18:49:33Z

R/insert-ui.R

+#'
+#' @param immediate Whether \code{tab} should be immediately inserted into
+#'   the app when you call \code{insertTab}, or whether Shiny should wait until
+#'   all outputs have been updated and all observers have been run (default).


I'm curious: when would people want to run it immediately, and when would the want to wait for a flush?

This might've been inherited from insertUI. In that case you would want to wait for a flush for things that are like outputs, and run immediately for things that are like progress.

Yeah, it definitely came from there. This is actually something I wanted to ask you Joe, i.e. if it's necessary to keep this arg for the tab functions. From your reply above, I'm guessing "no"? In which case, I just always call session$onFlushed(callback, once = TRUE) instead of giving the user an option... I think this is what makes sense, but I just wanted to confirm

I don't have a strong preference either way.

I think I'll remove the argument then; it's more likely it would confuse people, rather than enlighten them... Let me know if you object, @wch

jcheng5 · 2017-07-14T18:51:06Z

R/insert-ui.R

+#' @param target The \code{value} of an existing \code{tabPanel()}, next to
+#'   which \code{tab} will be added. If \code{NULL},the \code{tab} will be
+#'   placed either as the first tab or the last tab, depending on the
+#'   \code{position} argument.


I would tweak the target=NULL case. You've got it like this:

If target is non-NULL, right means "to the right of the target".

If target is NULL, right means "to the right of everything".

This overloading of the meaning of position feels strange to me. It also means that there are two ways of putting a tab at the end using right: either use a NULL target, or use the current last tab as the target; but, there is no way to put a tab at the beginning, without changing the position to left.

The usual way I've seen this done is:

If target is non-NULL, right means "to the right of the target". (no change)

If target is NULL, right means "to the right of nothing"--that is, the leftmost position.

This way, the meaning of position is more consistent. And also both position="right" and "left" are capable of addressing any possible slot.

So, in your proposed way, what does left mean when target is NULL? Is it "to the left of nothing", meaning the rightmost tab? Or is it "to the left of nothing", meaning the leftmost tab just the same?

Regardless, I also wanted to point out something. I chose to use this combination of target and position keeping in mind that the most important things would be:

All possible positions must be covered

The most typical use cases should be the most intuitive. There isn't much challenging about position if target is non-NULL. However, if target is NULL (or whatever value we decide to give to mean that the target is the whole tabset), then we want to allow users to quickly specify whether they want to put the tab at the beginning or at the end (which I think are the only two reasonable options when dealing with the whole tabset as the "target"). So, in this way, right means "to the right of all other tabs" and left means to the "left of all other tabs". This is particularly important when you want to keep either prepending or appending tabs, but you don't know (or don't want to keep track) of the tab value of the first or last tab, respectively.

So, my question is, under your scheme, is there a way that the user can prepend/append tabs without referring to specific tab names? That was the biggest consideration for me. So, while I agree that it is not ideal to have two ways of doing the same thing, I think I care more about the symmetry in being able to both prepend or append tabs without having to know what's the first (or last) tab value?

Let me know what you think :)

wch · 2017-07-14T18:58:42Z

srcjs/shinyapp.js

+      prevTabIds.push($(this).find('> a').attr('href').replace(leadingHref,''));
+    });
+    prevTabIds = prevTabIds.map(Number);
+    var tabId = Math.max.apply(Math, prevTabIds) + 1;


I think it would be slightly cleaner to use Math.max.apply(null, prevTabIds).

Done, thanks!

wch · 2017-07-14T19:07:34Z

Does this work for navbarPage, navbarMenu, and navlistPanel?

Also, for navbarPage and navbarMenu, it would be nice to be be able to add/remove from dropdown menus.

wch · 2017-07-14T19:26:47Z

srcjs/shinyapp.js

+      throw 'There is no tabsetPanel with id ' + message.tabsetPanelId;
+    };
+
+    // This is the JS equivalent of the builtItem() R function that is used


It's unfortunate that we have one copy of this code in R and another copy of it in JS.

I've actually never liked that tabsetPanels modify their contents on the R side. It might make sense to refactor all that code so that the buildTabset function is eliminated from the R side, and implemented in JS. To me, that feels like the right place to do the DOM mutations, and it would also allow the code to be reused here.

I agree. This was the most intuitive way of coding for me (i.e. try to understand previous code and work with it, rather than refactoring), but in hindsight, that may have been a better bet from the start. @jcheng5, what do you think? Is it worth it?

I'm not so familiar with buildTabset but I assume the answer to Winston's other question about navbarPage, navbarMenu, and navlistPanel would be a factor?

Alternatively, could all the rendering be done in R, not JS?

I assume the answer to Winston's other question about navbarPage, navbarMenu, and navlistPanel would be a factor

It is and it isn't. So, in answer to @wch's question about whether this works for navbarPage, navbarMenu, and navlistPanel, the answer is no. I guess I assumed tabsetPanel was the only tabset-thingie that was worth it to implement this for (i.e. the one that people reasonably expect to be able to add and remove tabs). Maybe that's wrong. And this is where we get to @jcheng5's point. Accommodating these scenarios would involve changing my JS (since I only implemented the tabsetPanel case). This would again be mostly a translation of the R code.

So, if we want to accommodate navbarPage, navbarMenu, and navlistPanel, I think we really have to stop the code duplication and settle on a place/language to put this in.
Like Winston, I think it makes more sense for this to be in the JS side, since that language is perfect for DOM mutations, whereas R isn't.

@wch , @jcheng5, should I go ahead with that? I think the whole refactoring + navbarPage, navbarMenu, and navlistPanel implementations + code review would likely take one more day (assuming all other feedback on the PR is already here).

jcheng5 · 2017-07-17T19:53:23Z

srcjs/shinyapp.js

@@ -730,7 +730,7 @@ var ShinyApp = function() {
      prevTabIds.push($(this).find('> a').attr('href').replace(leadingHref,''));
    });
    prevTabIds = prevTabIds.map(Number);
-    var tabId = Math.max.apply(Math, prevTabIds) + 1;
+    var tabId = Math.max.apply(null, prevTabIds) + 1;


What's this? Isn't Math correct?

I thought so too (see https://stackoverflow.com/a/1379556/6174455), and it certainly does the same thing. However, looking at the docs (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/max), you see they indeed use null instead of Math (code chunk from the second example):

function getMaxOfArray(numArray) { return Math.max.apply(null, numArray); }

bborgesr · 2017-07-22T07:12:28Z

Use this app for quick testing of the current work: https://beta.rstudioconnect.com/content/2917/

Insertion of items into tabsetPanel/navbarPage/navlistPanel is feature complete (this also includes navbarMenus as a special case of navbarPage). I still have to go over the feedback here and address another point that I didn't get to yet (making sure the tabset panel navigates naturally to another tab if the currently selected tab is hidden or removed).

I'm planning on taking a look at this later today, so I can leave it as perfect as possible for the code review next week. In any case, the vast, vast majority of this PR is just insertion, so that's why I prioritized that.

Update (7/24):
I think this PR is now feature complete. I updated the PR description above with the goal of having that be an overview of the new functionality. The last nit that I can’t seem to solve is that after hiding/removing the last tab on a tabsetPanel (or equivalent), while the UI looks as expected (no tabs), the value of input$tabs stays on whatever the last tab was, even though I’d think the getValue function would returnnull in that case.

jcheng5

HTML dependencies that are added by appending/inserting/prepending don't work.

library(leaflet)
library(shiny)

ui <- navbarPage(id = "nav", "App")

server <- function(input, output, session) {
  appendTab("nav", tabPanel("Map",
    "Map:",
    leaflet() %>% addTiles()
  ))
  session$onFlushed(function() {
    updateTabsetPanel(session, "nav", "Map")
  })
}

shinyApp(ui, server)

jcheng5

The "select nearest tab when removing currently active tab" seems not to work if the currently active tab is in a submenu:

library(shiny)

ui <- navbarPage("Tab removal testing", id = "nav",
  tabPanel("A", "This is A"),
  navbarMenu("B",
    tabPanel("C", "This is C"),
    tabPanel("D", "This is D")
  ),
  selected = "C",
  footer = actionButton("remove", "Remove C")
)

server <- function(input, output, session) {
  observeEvent(input$remove, {
    removeTab("nav", "C")
  })
}

shinyApp(ui, server)

I'd expect hitting the "Remove C" action button to cause tab D to be selected.

I talked to Jonathan McPherson about different strategies for selecting tabs when the current tab goes away. We came up with four reasonable strategies:

Activate the "nearest" tab. Turns out the logic is surprisingly complex. I don't think the cases enumerated in the code currently are all of them--if you remove the last node in a submenu you'd need to go up to the parent level.
Activate the most-recently-active, and still visible, tab. This is what RStudio does.
Always activate the first tab. (Breadth-first or depth-first traversal?)
"Secret parent"--at the time that the tab is (dynamically) added, take note of which tab is currently active, and record a property on the new tab that the currently active tab is its secret parent. This would be reasonable if the current tab has a button or something on it that causes new tabs to be created; when you close such tabs, you jump back to where you can make new ones.

I don't know how to feel about all of these options. Tempting to just do "first" and be done with it, for now; and add a parameter later if we want users to be able to be more specific about the behavior they want.

Update 2017-08-01: Another half-solution is to have an implicit input that contains the names of all the tabs (and submenus and their tabs). This would at least make it easier for the user to figure out at runtime which tab should be selected (e.g. if they don't keep track of the names of tabs they dynamically insert). Like selecting the last tab would be updateTabsetInput(session, "tabset", selected = tail(input$tabset_tabvalues, 1). However this is still not that great because input$xxx_tabvalues would always reflect the set of tabs at the time the last message was received by the client--you might've already done removals or whatever since then.

jcheng5 · 2017-07-27T17:38:14Z

It's a bit unfortunate that the new functions all delay until flushed, while updateTabsetInput happens immediately. So insertTab followed immediately by changing the selection doesn't work--you have to wrap the latter in session$onFlushed. (It's actually fine for removeTab because it's fine to switch tabs before removing.)

Let's have append/prepend/insert take another argument, selected = FALSE.

jcheng5 · 2017-07-28T09:18:51Z

I've addressed the dependencies not loaded, and added a select parameter to insert, prepend, and append.

Still need to revisit the logic that fires when the currently active tab is removed/hidden. I talked to Winston about this today and we thought about what kinds of edge cases real-world uses are likely to run into, and we decided to do the simplest thing for now, which is to jump to the first tab every time.

jcheng5 · 2017-07-28T17:59:26Z

We need these test cases for anywhere we insert dynamic UI:

<script> blocks should run
<script> blocks should only run once
head()/singleton() should be respected
HTML widgets should work
a. Even when the dependencies are not part of the initial page load
Shiny inputs/outputs should work
Subapps should work (include a shinyApp object right in the UI)

I'm pretty sure at least number 1 is failing for insertUI where the position is not the default "replace"?

jcheng5 · 2017-07-29T01:18:51Z

All outstanding issues^* that I know of are fixed. I'd like to code review with @bborgesr and either @wch or @alandipert on Monday, then we can make a decision about whether we think this is safe to merge now, or whether we should delay merging until right after we go to CRAN.

^* with the tab functionality--not the insertUI script block issue from my last comment, I'm not touching that for now

bborgesr · 2017-08-04T13:59:31Z

Modules test app:

library(shiny)

modUI <- function(id) {
  ns <- NS(id)
  
  tagList(
    actionButton(ns("insert"), "Test insert + update"),
    actionButton(ns("show"), "Test show + update"),
    actionButton(ns("update"), "Update tabset"),
    tabsetPanel(id = ns("tabs"),
      tabPanel("First", "First tab panel"),
      tabPanel("Second", "Second tab panel")
    )
  )
}

mod <- function(input, output, session) {
  hideTab("tabs", "Second")
  
  observeEvent(input$insert, {
    appendTab("tabs", tabPanel("Dynamic", "Dynamic Tab"), select = TRUE)
  })
  
  observeEvent(input$show, {
    showTab("tabs", "Second", select = TRUE)
  })
  
  observeEvent(input$update, {
    updateTabsetPanel(session, "tabs", "Second")
  })
}


ui <- fluidPage(
  modUI("module")
)

server <- function(input, output, session) {
  callModule(mod, "module")
}

shinyApp(ui, server)

…ion$onFlushed`)

…de a navbarMenu (or the whole menu) and it is selected (before this commit, it wasn't navigating to the first tab like it is supposed to)

jcheng5 · 2017-08-04T16:11:43Z

R/shiny.R

@@ -846,6 +846,17 @@ ShinySession <- R6Class(
        registerDataObj = function(name, data, filterFunc) {
          .subset2(self, "registerDataObj")(ns(name), data, filterFunc)
        },
+        sendInsertTab = function(inputId, liTag, divTag, menuName,
+                                 target, position, select) {
+          .subset2(self, "sendInsertTab")(ns(inputId), liTag, divTag, 


Instead of reimplementing these, can you just have the "real" implementations wrap the inputId with session$ns()?

jcheng5 · 2017-08-04T16:45:02Z

srcjs/shinyapp.js

  function ensureTabsetHasVisibleTab($tabset) {
-    if ($tabset.find("li.active").length === 0) {
+    if ($tabset.find("li.active").not(".dropdown").length === 0) {


Ah, I didn't even realize a dropdown could be active

jcheng5 · 2017-08-04T16:49:39Z

You shouldn't change this now in any case, but in the future, I'd avoid making arrays of jQuery objects and then iterating over their contents (like in getTargetTabs and tabApplyFunction). jQuery objects are already array-like and apply methods to all the DOM objects they represent.

…le functionality (rather than overriding the same functions in `makeScope`)

bborgesr · 2017-08-04T17:20:18Z

@jcheng5 Re your last comment, I did start that way (the each function itself is a lot better suited for jQuery objects than than regular arrays), but I couldn't get it to work the way iIneeded it to with a naive approach... I'll definitely re-try it once this is safely in master and not blocking our release. I also think there must be a much better way of writing those loops...

keqiang · 2017-08-09T16:52:17Z

I just tried using appendTab to dynamically add tabPanels but it always displays the content of the first added tabPanel although I set 'select' to TRUE. The prepend has the same issue.

ui <- fluidPage(
  sidebarLayout(
    sidebarPanel(
      actionButton("add", "Add 'Dynamic' tab")
    ),
    mainPanel(
      tabsetPanel(id = "tabs")
    )
  )
)

server <- function(input, output) {
  
  observeEvent(input$add, {
    appendTab(
      inputId = "tabs",
      tab = tabPanel("Dynamic", paste("This a dynamically-added tab", input$add)),
      select = TRUE
    )
  })
}

shinyApp(ui, server)

wch · 2017-08-09T17:16:43Z

@keqiang I think the problem is that you are using appendTab when there is no existing tab. If you have one tab to begin with, it should work. That said, I think appendTab should work when there are no existing tabs. Can you open a new issue for this?

keqiang · 2017-08-09T17:21:23Z

@wch Thanks! Yes, I noticed it happens only when it's empty, but I feel a lot of times I will programmatically add all my tabs and to me append shouldn't require any existing element. I will open a new issue.

bborgesr added the review label Jul 14, 2017

wch reviewed Jul 14, 2017

View reviewed changes

jcheng5 reviewed Jul 14, 2017

View reviewed changes

wch reviewed Jul 14, 2017

View reviewed changes

jcheng5 reviewed Jul 17, 2017

View reviewed changes

jcheng5 requested changes Jul 25, 2017

View reviewed changes

bborgesr added 7 commits August 4, 2017 15:09

changes

20bff18

add functions to index.r

72a1b3d

finish insertTab

b94efe8

implement removeTab

fb7b6f6

implement showTab and removeTab

b6b6661

delete extra brackets

a409562

update examples

2813e0b

bborgesr force-pushed the barbara/tabs branch from 3582d63 to ee5da14 Compare August 4, 2017 14:10

bborgesr added 5 commits August 4, 2017 15:10

added NEWS and fixed typo

ae7f026

clean up JS code (1 line only)

0ecdcec

now working for navbarPage and navlistPanel

a4c086f

document similar things together; add prependTab and appendTab

3adbebc

Fixed documentation problems and JS code logic

1ecc9b9

bborgesr and others added 15 commits August 4, 2017 15:10

updated docs

e660278

refactored code and made insertion of navbarMenus possible

0e7c78b

insertion fully implemented

d57aa33

more JS code refactoring; improved documentation

10f3320

implement navigation after hiding/removing selected tab

6f9f3fe

Properly escape jQuery selector strings

48b8923

Make tab prepend/append just edge cases of insert

f1873a0

Add select=FALSE argument to insert/append/prependTab

dde7b14

htmlDependencies are properly loaded with dynamic tabs

91dbb0e

Fix bug where last tab being removed, didn't update tabset input value

c090efd

Abandon nearest neighbor tab-showing logic. Just grab the first tab.

c1c5873

for dynamic tabs, send message on session$onFlush (instead of `sess…

40cfff3

…ion$onFlushed`)

added select argument to showTab function

82ac112

make this PR work for modules

494627c

make hide and remove work well when we want to hide/remove a tab insi…

ee5da14

…de a navbarMenu (or the whole menu) and it is selected (before this commit, it wasn't navigating to the first tab like it is supposed to)

jcheng5 reviewed Aug 4, 2017

View reviewed changes

jcheng5 approved these changes Aug 4, 2017

View reviewed changes

bborgesr added 2 commits August 4, 2017 17:56

Joe's feedback

ed3ba30

do inputId <- session$ns(inputId) in user facing functions for modu…

b02eb11

…le functionality (rather than overriding the same functions in `makeScope`)

jcheng5 merged commit 3b0c390 into master Aug 4, 2017

jcheng5 deleted the barbara/tabs branch August 4, 2017 18:31

wch mentioned this pull request Aug 8, 2017

Feature request: dynamically add/remove tabs to an existing tabset #1668

Closed

This was referenced Jan 24, 2025

fix(insertTab): Render inserted nav html only once #4179

Merged

tests: Add 317-nav-insert rstudio/shinycoreci#279

Merged

Dynamic tabs #1794

Dynamic tabs #1794

Conversation

bborgesr commented Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcheng5 Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

bborgesr Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wch commented Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bborgesr commented Jul 22, 2017 • edited Loading

jcheng5 left a comment

Choose a reason for hiding this comment

jcheng5 left a comment • edited Loading

Choose a reason for hiding this comment

jcheng5 commented Jul 27, 2017 • edited Loading

jcheng5 commented Jul 28, 2017

jcheng5 commented Jul 28, 2017

jcheng5 commented Jul 29, 2017 • edited Loading

bborgesr commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcheng5 commented Aug 4, 2017

bborgesr commented Aug 4, 2017

keqiang commented Aug 9, 2017 • edited Loading

wch commented Aug 9, 2017

keqiang commented Aug 9, 2017

bborgesr commented Jul 14, 2017 •

edited

Loading

jcheng5 Jul 14, 2017 •

edited

Loading

bborgesr Jul 17, 2017 •

edited

Loading

wch commented Jul 14, 2017 •

edited

Loading

bborgesr commented Jul 22, 2017 •

edited

Loading

jcheng5 left a comment •

edited

Loading

jcheng5 commented Jul 27, 2017 •

edited

Loading

jcheng5 commented Jul 29, 2017 •

edited

Loading

keqiang commented Aug 9, 2017 •

edited

Loading