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

samelliottdlt · 2016-07-19T04:24:12Z

It is possible that _getTabs returns an array of undefined when mapping over linkArray as .controller('mdNavItem') can return undefined if it cannot find the corresponding controller which will cause an error to be shown in the console during the digest cycle.

This change assures that we do not attempt to update the tabs in the case that tabs is an undefined array.

googlebot · 2016-07-19T04:24:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-07-19T04:24:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

samelliottdlt · 2016-07-19T04:26:24Z

Signed CLA

googlebot · 2016-07-19T04:26:25Z

CLAs look good, thanks!

googlebot · 2016-07-19T04:26:25Z

CLAs look good, thanks!

ThomasBurleson · 2016-07-19T12:18:59Z

Please provide 1-2 unit tests for these. Thx @samdlt .

googlebot · 2016-07-20T21:24:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-07-20T21:24:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-07-20T22:20:30Z

CLAs look good, thanks!

googlebot · 2016-07-20T22:20:30Z

CLAs look good, thanks!

samelliottdlt · 2016-07-21T15:25:57Z

@ThomasBurleson added unit tests

clshortfuse · 2016-09-09T18:50:49Z

src/components/navBar/navBar.js

-    oldTab.setSelected(false);
-    oldIndex = tabs.indexOf(oldTab);
-  }
+  if (tabs) {


To prevent nesting, we can change this to if (!tabs) return;. We can also move it up to before oldIndex and newIndex are initialized.

yes, can you return immediately and as soon as possible?

clshortfuse · 2016-09-09T22:37:35Z

Fixes #9386

Can you add that to the commit message?

gmoothart · 2016-09-09T22:45:49Z

src/components/navBar/navBar.js

+      .map(function(el) {
+          return angular.element(el).controller('mdNavItem')
+      });
+    return linkArray.indexOf() === -1 ? linkArray : null;


linkArray.indexOf(undefined) reads a little better to me.

controllers instead of linkArray plz.

samelliottdlt · 2016-09-10T15:53:24Z

@gmoothart @clshortfuse Thanks for taking a look at this guys. I'll address the comments either tomorrow or Monday.

gmoothart

Looks good, pending some minor comments.

gmoothart · 2016-09-15T16:03:26Z

src/components/navBar/navBar.js

@@ -209,6 +209,10 @@ MdNavBarController.prototype._initTabs = function() {
 MdNavBarController.prototype._updateTabs = function(newValue, oldValue) {
  var self = this;
  var tabs = this._getTabs();
+
+  if(!tabs)
+    return;


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

ThomasBurleson · 2016-09-15T19:44:21Z

@samdlt - FYI, we will not merge until the review feedback is addressed and the PR refreshed with updates.

gmoothart

👍

gmoothart · 2016-09-15T22:27:04Z

src/components/navBar/navBar.js

@@ -209,6 +209,11 @@ MdNavBarController.prototype._initTabs = function() {
 MdNavBarController.prototype._updateTabs = function(newValue, oldValue) {
  var self = this;
  var tabs = this._getTabs();
+
+  // this._getTabs can return an array of undefined if nav-bar has not yet been initialized


(nit) technically it returns null, not an array of undefined.

gmoothart · 2016-09-27T16:44:23Z

@samdlt can you resolve the merge conflicts and then we can merge this?

samelliottdlt · 2016-10-03T21:01:56Z

@gmoothart merged and resolved conflict

jelbourn · 2016-12-02T23:32:04Z

@samdlt can you rebase?

samelliottdlt · 2016-12-06T23:05:55Z

@jelbourn rebased

ThomasBurleson added the needs: unit tests This PR needs unit tests to cover the changes being proposed label Jul 19, 2016

samelliottdlt force-pushed the master branch from 8e9284b to a8cdde7 Compare July 20, 2016 22:20

ThomasBurleson added the needs: group review label Sep 7, 2016

topherfangio assigned bradrich and clshortfuse Sep 9, 2016

topherfangio added needs: review This PR is waiting on review from the team and removed needs: group review needs: unit tests This PR needs unit tests to cover the changes being proposed labels Sep 9, 2016

clshortfuse reviewed Sep 9, 2016
View reviewed changes

clshortfuse mentioned this pull request Sep 9, 2016

fix(navBar): Validate tab controller is defined before calling find fn #9208

Closed

clshortfuse changed the title ~~Null check tabs in md-nav-bar~~ fix(nav-bar): null check tabs when updating nav-bar Sep 9, 2016

gmoothart reviewed Sep 9, 2016
View reviewed changes

gmoothart reviewed Sep 15, 2016

View reviewed changes

ThomasBurleson added needs: work in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Sep 15, 2016

ThomasBurleson added this to the 1.1.2 milestone Sep 15, 2016

gmoothart approved these changes Sep 20, 2016

View reviewed changes

gmoothart mentioned this pull request Sep 26, 2016

md-nav-bar throws error when used with md-nav-sref. #9386

Closed

ThomasBurleson added needs: presubmit and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Oct 9, 2016

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Dec 6, 2016

fix(nav-bar) null check tabs when updating nav-bar

fcba946

samelliottdlt force-pushed the master branch from b543e46 to fcba946 Compare December 6, 2016 23:05

kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Jan 3, 2017

kara merged commit b38d928 into angular:master Jan 3, 2017

davidenochk pushed a commit to davidenochk/material that referenced this pull request Feb 3, 2017

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

9775a61

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

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

Uh oh!

Conversation

samelliottdlt commented Jul 19, 2016

Uh oh!

googlebot commented Jul 19, 2016

Uh oh!

googlebot commented Jul 19, 2016

Uh oh!

samelliottdlt commented Jul 19, 2016

Uh oh!

googlebot commented Jul 19, 2016

Uh oh!

googlebot commented Jul 19, 2016

Uh oh!

ThomasBurleson commented Jul 19, 2016

Uh oh!

googlebot commented Jul 20, 2016

Uh oh!

googlebot commented Jul 20, 2016

Uh oh!

googlebot commented Jul 20, 2016

Uh oh!

googlebot commented Jul 20, 2016

Uh oh!

samelliottdlt commented Jul 21, 2016

Uh oh!

clshortfuse Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

gmoothart Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

clshortfuse commented Sep 9, 2016

Uh oh!

gmoothart Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasBurleson Sep 10, 2016

Choose a reason for hiding this comment

Uh oh!

samelliottdlt commented Sep 10, 2016

Uh oh!

gmoothart left a comment

Choose a reason for hiding this comment

Uh oh!

gmoothart Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasBurleson commented Sep 15, 2016

Uh oh!

gmoothart left a comment

Choose a reason for hiding this comment

Uh oh!

gmoothart Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

gmoothart commented Sep 27, 2016

Uh oh!

samelliottdlt commented Oct 3, 2016

Uh oh!

jelbourn commented Dec 2, 2016

Uh oh!

samelliottdlt commented Dec 6, 2016

Uh oh!

Uh oh!