Closed bborgesr closed 7 years ago
I'm having a bit of a hard time following the logic in the code... I could be missing something, but can all that code be reduced to this?
var $startTab = $tablinks.filter("[data-start-selected='1']");
if ($startTab.length === 0) {
// If no tab starts selected, use the first one, if present
$startTab = $tablinks.first();
}
// If there are no tabs, $startTab.length will be 0.
if ($startTab.length !== 0) {
$startTab.tab("show");
$(".sidebarMenuSelectedTabItem").attr("data-value",
$startTab.attr("data-value"));
}
The things I'm not sure about are:
$startTab.length
will be 0 when there are no tabs?$tablinks.parent("li").hasClass("active")
, and I've removed it in this version. Is there a problem with removing that check?As for the logic, I think you're completely right. I was working off existing code and I basically built up on it, and didn't take a step back to try and frame it in a more coherent piece of code. I think your code is spot on.
Am I right that
$startTab.length
will be 0 when there are no tabs?
Yes, although this is also technically and briefly true for dynamic sidebarMenu()
s, since ensureActivatedTab()
is called twice for those apps. We may consider this a small performance issue, but I think it's negligible and this new code certainly isn't adding to the "problem" -- once the menu is rendered in the server function, ensureActivatedTab()
is called again and the input value (for input$tabs
) is then correctly set.
As for the check $tablinks.parent("li").hasClass("active")
, it can be traced back to this commit from 2014. I think I've never questioned that, but now that you mention it, I do think we can remove it. It seems that in the original context, the point of the check was just to make sure that we're not showing something that is already shown..? From my testing now (for all the variations of the repro and some quick testing on larger apps), I don't think there's any issue. (Also, logically, I don't see why there should be a problem.)
I've pushed your code, after some testing. I think we're good to go..?
Now that I look at it some more, I think that $tablinks.parent("li").hasClass("active")
might have
something to do with menuSubItems, like when the first menuItem has menuSubItems.
I think this behaves the same as before, but there's some weird behavior with the tab1 menuItem. It doesn't start expanded, and also, when it is expanded and you click on tab1_1, the little arrow on the right changes from pointing down to pointing left. But that's sort of a separate issue, and fixing those things could be a separate PR if you prefer.
library(shinydashboard)
library(shiny)
ui <- dashboardPage(
dashboardHeader(),
dashboardSidebar(
sidebarMenu(id = "tabs",
menuItem("tab1",
menuSubItem("tab1_1", tabName = "tab1_1")
),
menuItem("tab2", tabName = "tab2")
)),
dashboardBody(
tabItems(
tabItem("tab1", "This is tab 1"),
tabItem("tab1_1", "This is tab 1_1"),
tabItem("tab2", "This is tab 2")
)
)
)
server <- function(input, output, session){
observe({ print(input$tabs) })
}
shinyApp(ui, server)
Fixes #214: make sure that the
data-value
attribute of.sidebarMenuSelectedTabItem
is always set in the body of theensureActivatedTab()
function.Repro
See #214 or here is a more concise one (bug:
input$tabs
should start out with the value"tab1"
, but it starts out withNULL
and stays that way until you click on the other tab):Postemortem
This bug was hard to spot for a few reasons:
selected = TRUE
on any tab (the default). I.e. this has the right behavior:It disappears as soon as you move on to another tab.
It's orthogonal to the matching up of
menuItem
s andtabItem
s. I.e. this works works as expected, even thoughinput$tabs
starts out NULL as well: