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

fix(tab): pass the tabs real index to onDeselect #6113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

furti
Copy link

@furti furti commented Jul 22, 2016

The $selectedIndex passed to the onDeselect callback is the same as the
index given in the tab template. One can use that index as it is for the
active value of the tabset.

Fixes #6107

The $selectedIndex passed to the onDeselect callback is the same as the
index given in the tab template. One can use that index as it is for the
active value of the tabset.

Fixes angular-ui#6107
@furti
Copy link
Author

furti commented Jul 22, 2016

I'm not sure if this is the right place for this but i have a question for a test case in tabs.spec.js

The test case is as follows:

it('should prevent tab deselection when $event.preventDefault() is called', function() {
      spyOn(scope, 'deselectThird');
      titles().eq(2).find('> a').click();
      expect(scope.active).toBe(3);
      titles().eq(1).find('> a').click();
      expect(scope.deselectThird).toHaveBeenCalled();
      expect(scope.active).not.toBe(1);
      expect(scope.active).toBe(2);
    });
  1. create spy -> OK
  2. select the third tab -> OK
  3. the active tab should be the third one -> OK
  4. Select the second tab -> OK
  5. The deselectThird callback should have been called -> OK
  6. the active tab should not be the first one -> Confusing. The first tab was not even involved in this whole scenario because we tried to switch from the third tab to the second one. Why is this checked here?
  7. the active tab should be the second one -> Confusing. The deselectThird callback prevents the default action. So the active tab should still be the third one as the tab change got cancelled.

@wesleycho
Copy link
Contributor

Sorry for the long silence here - I have been on vacation for the past 2 weeks in Europe, and return home this upcoming weekend. I will look at this & the original issue in more detail next week.

@wesleycho
Copy link
Contributor

Scheduling this for 3.0.0 - this is a breaking change, so as per semver, we'll have to hold off.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants