Conversation
As discussed on Gitter
|
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. |
|
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 |
Current coverage is 82.68% (diff: 85.18%)@@ 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
|
| 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 />'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" notifications@github.com
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
.
|
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! |
| scope.$on('$destroy', function() { | ||
| $rootElement.removeClass('has-navbar-' + side); | ||
| }); | ||
| if (!global) global = ctrl; |
There was a problem hiding this comment.
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;
|
Basically. But I don't limit it to that. You should be able to inject the On Sep 7, 2016 3:16 PM, "Maurizio Casimirri" notifications@github.com
|
|
Another example of why you would want to have an all knowing controller. Orientation changes. |
3 participants
As discussed on Gitter