feat(tabs): makes deselect callback fire before select. fixes #1557. #1566
Conversation
| angular.forEach(tabs, function(tab) { | ||
| tab.active = false; | ||
| if(tab.active && tab !== selectedTab) { |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, you are right about the 2 active tabs, so the double check is needed.
|
@jroxendal Left some comments on the code. This is on the right track but still needs some work. Also, about the tesst you should create a separate |
|
@jroxendal would be awesome if you could address @bekos comments so this one can be landed. |
|
@pkozlowski-opensource i'm on the case! :) |
|
@bekos I've made the changes you suggested. Please let me know if any further changes are necessary. Just an observation: currently, if the first tab has been explicitly marked as active=false, it's still set to active=true and its onselect callback i fired. I'd consider this a bug but didn't supply a fix since fixing this means that the tabs widget could be in a state where no tabs are active. It's pretty easy for a user to avoid that state, of course: just don't set active=false on the first tab. Any thoughts on what the correct behavior should be? |
| @@ -153,6 +152,56 @@ describe('tabs', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('tab callback order', function() { | |||
| var execOrder = []; | |||
There was a problem hiding this comment.
As I said, you don't have to initialize this here, just leave the declaration.
You have to do it inside the beforeEach, so each test does not mess with previous execution orders.
|
@jroxendal I like your changes! Also left some comments about some small things into the test code. |
|
@bekos thanks for the comments, I'll make the relevant changes in the morning! |
|
@bekos I've followed your advice, it's a lot neater now :) |
|
@jroxendal Nice one :-) Thx! |
|
@bekos My pleasure, thank you for the code review! |
This should fix the call order of the select and deselect callbacks.