Nav Controller

[ballenjr]

As discussed on Gitter

[ballenjr]

I'm torn...

Should I make the require: "navController" optional, and check for it, or should I assume that ballenjr/generator-mobileangularui#1 will take care of it?

I am thinking that the navController should always be bound. But if that is not agreeable then I will make it an optional check, it shouldn't hurt anything else if it's not bound. What do you think @mcasimir ?

To be clear, I am saying that I am assuming that the tests that fail are using the template from the generator which does not yet have ballenjr/generator-mobileangularui#1 applied.

[ballenjr]

I have no idea what happened. I can't get it to make the PR for your repo. Shouldn't be a big deal though. It's just 2 simple changes. ballenjr/generator-mobileangularui#1

[codecov-io]

Current coverage is 82.68% (diff: 85.18%)

Merging #378 into master will increase coverage by 2.43%

@@             master       #378   diff @@
==========================================
  Files            14         14          
  Lines           562        589    +27   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            451        487    +36   
+ Misses          111        102     -9   
  Partials          0          0          

Powered by Codecov. Last update 0a8030c...29f5b11

[mcasimir]

That's cheating! :D

That kind of coverage is useless, coverage is just a number after all, and unit tests requires isolation to be reliable.

[ballenjr]

Ya I didn't think that it would be very useful either. I was just using the
other test as a guide.

On Sep 7, 2016 2:22 PM, "Maurizio Casimirri" [email protected]
wrote:

In test/unit/components/navbars.spec.js
#378 (comment)
:

@@ -21,15 +21,15 @@ describe('components', function() {

 describe('navbarAbsoluteTop', function() {
   it('adds class has-navbar-top if navbarAbsoluteTop is present', function() {

•    let elem = angular.element('<div class="navbar-absolute-top" />');
  

•    let elem = angular.element('<div class="navbar-absolute-top" nav-controller />');
  

That's a nice trick :D

That kind of coverage is useless, coverage is just a number after all, and
unit tests requires isolation to be reliable.

Try to write a test suite for your feature that works independently from
the rest.

Or leave it as it was and i'll try to cover it.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/mcasimir/mobile-angular-ui/pull/378/files/ccdae045247c4f5e44c42d1cbd5319eb3fcb252f#r77906493,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADDzgw1gTJQQaYoHcGATnBh5u1VZH_6Oks5qnysbgaJpZM4Jx8nS
.

[mcasimir]

Hi @ballenjr, ok sorry to took so long to review your code and thank you very much to share it.

Is still a good point to start but currently the way it is i'd like to reject it, cause i believe we can find together a better approach and solution.

What i'm missing is.. in response to what you want the navbar to show/hide? Depending on the page? It shows and than goes away and that's it? ..

Thanks!

[mcasimir]

No one liners without brackets pls, although this one would be a ligit one for conciseness.

if (!global) {
  global = ctrl;
}

I'd favor this form

global = global || ctrl;

[ballenjr]

Basically. But I don't limit it to that. You should be able to inject the
controller at will, I think.

On Sep 7, 2016 3:16 PM, "Maurizio Casimirri" [email protected]
wrote:

Hi @ballenjr https://github.com/ballenjr, ok sorry to took so long to
review your code and thank you very much to share it.

Is still a good point to start but currently the way it is i'd like to
reject it, cause i believe we can find together a better approach and
solution.

What i'm missing is.. in response to what you want the navbar to
show/hide? Depending on the page? It shows and than goes away and that's
it? ..

Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#378 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADDzgzgKfhpH3TdhuNbJNxPhz9Kc7W1fks5qnzfDgaJpZM4Jx8nS
.

[ballenjr]

#359

Another example of why you would want to have an all knowing controller. Orientation changes.