Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(nav-bar): null check tabs when updating nav-bar #9071

Merged
merged 1 commit into from
Jan 3, 2017
Merged

fix(nav-bar): null check tabs when updating nav-bar #9071

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
16 changes: 11 additions & 5 deletions src/components/navBar/navBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,11 @@ MdNavBarController.prototype._initTabs = function() {
MdNavBarController.prototype._updateTabs = function(newValue, oldValue) {
var self = this;
var tabs = this._getTabs();

// this._getTabs can return null if nav-bar has not yet been initialized
if(!tabs)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here explaining why tabs might be null?


var oldIndex = -1;
var newIndex = -1;
var newTab = this._getTabByName(newValue);
Expand Down Expand Up @@ -266,11 +271,12 @@ MdNavBarController.prototype._updateInkBarStyles = function(tab, newIndex, oldIn
* @private
*/
MdNavBarController.prototype._getTabs = function() {
var linkArray = Array.prototype.slice.call(
this._navBarEl.querySelectorAll('.md-nav-item'));
return linkArray.map(function(el) {
return angular.element(el).controller('mdNavItem');
});
var controllers = Array.prototype.slice.call(
this._navBarEl.querySelectorAll('.md-nav-item'))
.map(function(el) {
return angular.element(el).controller('mdNavItem')
});
return controllers.indexOf(undefined) ? controllers : null;
};

/**
Expand Down
11 changes: 11 additions & 0 deletions src/components/navBar/navBar.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,17 @@ describe('mdNavBar', function() {
expect(getTab('tab2').attr('ui-sref-opts'))
.toBe('{"reload":true,"notify":true}');
});

it('does not update tabs if tab controller is undefined', function() {
$scope.selectedTabRoute = 'tab1';

spyOn(Object.getPrototypeOf(ctrl), '_updateInkBarStyles');
Copy link
Contributor

Choose a reason for hiding this comment

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

is Object.getPrototypeOf necessary? I'm surprised you can't just do spyOn(ctrl, '_upateInkBarStyles');

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure the Object.getPrototypeOf indirection is unnecessary. Can you remove the 3 instances in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to actually be necessary for some reason. Just ran a few tests:
spyOn(ctrl, '_upateInkBarStyles'); results in the test failing with this error: : Expected a spy, but got Function.
spyOn(ctrl.prototype, '_upateInkBarStyles'); also results in the test failing with this error: : could not find an object to spy upon for _updateInkBarStyles()

The expect can be modified to utilize the ctrl object instead and the test passes as expected.

Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for trying; this is ok.

spyOn(Object.getPrototypeOf(ctrl), '_getTabs').and.returnValue(null);
createTabs();

expect(ctrl._updateInkBarStyles)
.not.toHaveBeenCalled();
});
});

describe('inkbar', function() {
Expand Down