Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

feat(tabs): makes deselect callback fire before select. fixes #1557. #1566

Closed
wants to merge 1 commit into from
Closed

feat(tabs): makes deselect callback fire before select. fixes #1557. #1566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions src/tabs/tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,24 @@ angular.module('ui.bootstrap.tabs', [])
var ctrl = this,
tabs = ctrl.tabs = $scope.tabs = [];

ctrl.select = function(tab) {
ctrl.select = function(selectedTab) {
angular.forEach(tabs, function(tab) {
tab.active = false;
if(tab.active && tab !== selectedTab) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jroxendal Is there a way that this function will be called on already active tab? What I mean do we really need the double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the code is written now, this is necessary. On tab click, active is set to true. The active watch is then run, where this function is called and at this time two tabs may actually be active: the 'old' tab (which should be deselected) and the one we just selected. Only one of the deselect callbacks should be run, obviously.

I didn't want to do a complete rewrite, so I left the active watch in the tabs directive in there. In order to avoid the check above, I believe that watch needs to go and some further refractoring to take place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right about the 2 active tabs, so the double check is needed.

tab.active = false;
tab.onDeselect();
}
});
tab.active = true;
selectedTab.active = true;
selectedTab.onSelect();
};

ctrl.addTab = function addTab(tab) {
tabs.push(tab);
if (tabs.length === 1 || tab.active) {
// we can't run the select function on the first tab
// since that would select it twice
if (tabs.length === 1) {
tab.active = true;
} else if(tab.active) {
ctrl.select(tab);
}
};
Expand Down Expand Up @@ -206,9 +214,6 @@ angular.module('ui.bootstrap.tabs', [])
setActive(scope.$parent, active);
if (active) {
tabsetCtrl.select(scope);
scope.onSelect();
} else {
scope.onDeselect();
}
});

Expand Down
51 changes: 48 additions & 3 deletions src/tabs/test/tabs.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ describe('tabs', function() {
expect(titles().eq(0)).toHaveClass('active');
expect(titles().eq(1)).not.toHaveClass('active');
expect(scope.actives.one).toBe(true);
expect(scope.actives.two).toBe(false);
expect(scope.actives.two).not.toBeDefined();
});

it('should change active on click', function() {
Expand All @@ -99,7 +99,6 @@ describe('tabs', function() {
titles().eq(1).find('a').click();
expect(scope.deselectFirst).toHaveBeenCalled();
});

});

describe('basics with initial active tab', function() {
Expand Down Expand Up @@ -153,6 +152,48 @@ describe('tabs', function() {
});
});

describe('tab callback order', function() {
var execOrder;
beforeEach(inject(function($compile, $rootScope) {
scope = $rootScope.$new();
execOrder = [];
scope.actives = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

So you have to add: execOrder = [];


scope.execute = function(id) {
execOrder.push(id);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating multiple function to do the same, I would have just:

scope.execute = function(id) {
    execOrder.push(id);
};


elm = $compile([
'<div>',
' <tabset class="hello" data-pizza="pepperoni">',
' <tab heading="First Tab" active="actives.one" select="execute(\'select1\')" deselect="execute(\'deselect1\')"></tab>',
' <tab select="execute(\'select2\')" deselect="execute(\'deselect2\')"></tab>',
' </tabset>',
Copy link
Contributor

Choose a reason for hiding this comment

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

So here you can have it like:

'<tab heading="First Tab" active="actives.one" select="execute(\'select1\')" deselect="execute(\'deselect1\')"></tab>',
' <tab select="execute(\'select2\')" deselect="execute(\'deselect2\')"></tab>',

'</div>'
].join('\n'))(scope);
scope.$apply();
return elm;
}));

it('should call select for the first tab', function() {
expect(execOrder).toEqual([ 'select1' ]);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I would break this into two tests and I would also change the comparisons to make it more readable. Something like:

it('should call select  for the first tab', function() {
    expect(execOrder).toEqual([ 'select1' ]);
});

it('should call deselect, then select', function() {
      execOrder = [];

      // Select second tab
      titles().eq(1).find('a').click();
      expect(execOrder).toEqual([ 'deselect1', 'select2' ]);

      execOrder = [];

      // Select again first tab
      titles().eq(0).find('a').click();
      expect(execOrder).toEqual([ 'deselect2', 'select1' ]);
});


it('should call deselect, then select', function() {
execOrder = [];

// Select second tab
titles().eq(1).find('a').click();
expect(execOrder).toEqual([ 'deselect1', 'select2' ]);

execOrder = [];

// Select again first tab
titles().eq(0).find('a').click();
expect(execOrder).toEqual([ 'deselect2', 'select1' ]);
});
});

describe('ng-repeat', function() {

beforeEach(inject(function($compile, $rootScope) {
Expand Down Expand Up @@ -346,7 +387,11 @@ describe('tabs', function() {

describe('tabset controller', function() {
function mockTab(isActive) {
return { active: !!isActive };
return {
active: !!isActive,
onSelect : angular.noop,
onDeselect : angular.noop
};
}

var ctrl;
Expand Down