angular-ui / bootstrap

PLEASE READ THE PROJECT STATUS BELOW. Native AngularJS (Angular) directives for Bootstrap. Smaller footprint (20kB gzipped), no 3rd party JS dependencies (jQuery, bootstrap JS) required. Please read the README.md file before submitting an issue!
http://angular-ui.github.io/bootstrap/
MIT License
14.27k stars 6.73k forks source link

Wrong $selecteIndex value for deselect callback #6107

Open furti opened 8 years ago

furti commented 8 years ago

Bug description

The deselect callback can take the $selectedIndex as a parameter. But this is always the index of the tab in the internal array of the tabset. But it should be the index of the tab itself.

When the index attribute of a tab is specified I would assume that the $selectedIndex parameter of the deselect callback is the index specified on the tab.

Link to minimally-working plunker that reproduces the issue:

http://plnkr.co/edit/n2oXnJ5TqDQqfJBz5ur0?p=preview

Version of Angular, UIBS, and Bootstrap

Angular: 1.5.7

UIBS: 2.0.0

Bootstrap: 3.3.6

icfantv commented 8 years ago

@furti i'm not sure this makes sense until we support strings for tab indices.

furti commented 8 years ago

@icfantv are strings for tab Indices not supported right now? The documentation for Tabs says

index - Tab index. Must be unique number or string.

furti commented 8 years ago

I think something like this might work:

https://github.com/angular-ui/bootstrap/blob/master/src/tabs/tabs.js


ctrl.select = function(index, evt) {
    if(!destroyed) {
        var previousIndex = findTabIndex(oldIndex); 
        var previousSelected = ctrl.tabs[previousIndex]; 
        //Move this up from line 23
        var selected = ctrl.tabs[index];

        if (previousSelected) { 
            previousSelected.tab.onDeselect({ 
                $event: evt, 
                $selectedIndex: selected.index //Use the index of the selected tab here
            }); 
        ...
    }
}
icfantv commented 8 years ago

hmmm. i thought that issue got canned. i'll need to take a look at the code.

wesleycho commented 8 years ago

On superficial thought, this sounds correct - this should be an easy PR if you're up for filing one.

Edit: Shouldn't it be previousSelected.index there?

furti commented 8 years ago

@wesleycho I will check if i can make a PR for this fix. As this is my first PR for this project i will have to get the project up and running first.

I think it should be selected.index because the documentation says:

The $selectedIndex can be used to determine which tab was attempted to be opened

And the previous selected index could be easily passed in on the template:

<uib-tab index="entry.index" heading="{{entry.heading}}" 
    ng-repeat="entry in $ctrl.entries" deselect="$ctrl.tabDeselected(entry.index, $selectedIndex)">
    {{entry.content}}
</uib-tab>
niemyjski commented 8 years ago

I'm having an issue where the active index is being blown away when I reset the tabs... (vm.tabs = tabs... vm.activeindex = 2) and the watcher sets the index to null.

niemyjski commented 8 years ago

Not 100% sure if this is the same issue but I referenced it above.. There is an issue when tabs are removed (rebuilt) it resets the active index to null which happens after a watch (destroy) is called (in the future). This causes the index to be set to null even though it’s valid and new tabs have been added.