Nav Controller #378
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" 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 On Sep 7, 2016 2:22 PM, "Maurizio Casimirri" notifications@github.com
|
||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).toContain('has-navbar-top'); | ||
| }); | ||
|
|
||
| it('removes class has-navbar-top if navbarAbsoluteTop is destroyed', function() { | ||
| let elem = angular.element('<div class="navbar-absolute-top" nav-controller />'); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
| scope.$destroy(); | ||
|
|
@@ -39,20 +39,105 @@ describe('components', function() { | |
|
|
||
| describe('navbarAbsoluteBottom', function() { | ||
| it('adds class has-navbar-bottom if navbarAbsoluteBottom is present', function() { | ||
| let elem = angular.element('<div class="navbar-absolute-bottom" nav-controller />'); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).toContain('has-navbar-bottom'); | ||
| }); | ||
|
|
||
| it('removes class has-navbar-bottom if navbarAbsoluteBottom is destroyed', function() { | ||
| let elem = angular.element('<div class="navbar-absolute-bottom" nav-controller />'); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
| scope.$destroy(); | ||
| expect($rootElement.attr('class')).not.toContain('has-navbar-bottom'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('hideTopNav', function() { | ||
| it('hides the navbarAbsoluteTop when present within the nested view', function() { | ||
| let value = '<div><top class="navbar-absolute-top" nav-controller />'; | ||
| value += '<ng-view><div class="hide-top-nav"></div></ng-view></div>'; | ||
| let elem = angular.element(value); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).not.toContain('has-navbar-top'); | ||
| expect(elem.find('top').attr('class')).toContain('ng-hide'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('hideBottomNav', function() { | ||
| it('hides the navbarAbsoluteBottom when present within the nested view', function() { | ||
| let value = '<div><bottom class="navbar-absolute-bottom" nav-controller />'; | ||
| value += '<ng-view><div class="hide-bottom-nav"></div></ng-view></div>'; | ||
| let elem = angular.element(value); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).not.toContain('has-navbar-bottom'); | ||
| expect(elem.find('bottom').attr('class')).toContain('ng-hide'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('slideoutBottomNav', function() { | ||
| it('hides the navbarAbsoluteBottom when present within the nested view', function() { | ||
| let value = '<div><bottom class="navbar-absolute-bottom" nav-controller />'; | ||
| value += '<ng-view><div class="slideout-bottom-nav"></div></ng-view></div>'; | ||
| let elem = angular.element(value); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).not.toContain('has-navbar-bottom'); | ||
| expect(elem.find('bottom').attr('class')).toContain('ng-hide'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('slideoutTopNav', function() { | ||
| it('hides the navbarAbsoluteTop when present within the nested view', function() { | ||
| let value = '<div><top class="navbar-absolute-top" nav-controller />'; | ||
| value += '<ng-view><div class="slideout-top-nav"></div></ng-view></div>'; | ||
| let elem = angular.element(value); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).not.toContain('has-navbar-top'); | ||
| expect(elem.find('top').attr('class')).toContain('ng-hide'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('hideBothNav', function() { | ||
| it('hides both navbarAbsoluteTop && navbarAbsoluteBottom when present within the nested view', function() { | ||
| let value = '<div><bottom class="navbar-absolute-bottom" nav-controller />'; | ||
| value += '<top class="navbar-absolute-top" nav-controller /><ng-view>'; | ||
| value += '<div class="hide-both-nav"></div></ng-view></div>'; | ||
| let elem = angular.element(value); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).not.toContain('has-navbar-bottom'); | ||
| expect($rootElement.attr('class')).not.toContain('has-navbar-top'); | ||
| expect(elem.find('bottom').attr('class')).toContain('ng-hide'); | ||
| expect(elem.find('top').attr('class')).toContain('ng-hide'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('slideoutBothNav', function() { | ||
| it('hides both navbarAbsoluteTop && navbarAbsoluteBottom when present within the nested view', function() { | ||
| let value = '<div><bottom class="navbar-absolute-bottom" nav-controller />'; | ||
| value += '<top class="navbar-absolute-top" nav-controller /><ng-view>'; | ||
| value += '<div class="slideout-both-nav"></div></ng-view></div>'; | ||
| let elem = angular.element(value); | ||
| compile(elem)(scope); | ||
| scope.$digest(); | ||
|
|
||
| expect($rootElement.attr('class')).not.toContain('has-navbar-bottom'); | ||
| expect($rootElement.attr('class')).not.toContain('has-navbar-top'); | ||
| expect(elem.find('bottom').attr('class')).toContain('ng-hide'); | ||
| expect(elem.find('top').attr('class')).toContain('ng-hide'); | ||
| }); | ||
| }); | ||
|
|
||
| }); | ||
| }); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite too generic to be understood, perhaps a more specific name would be better, also
globalis kind of reserved namespace in node.js, that makes it even more confusing.What is it supposed to hold, why u need a singleton for that?
I've still to read the rest but that looks like a very disruptive approach.
We'll fix it anyway i like your idea of supporting common patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not my directive. It was there already.
On Sep 7, 2016 2:28 PM, "Maurizio Casimirri" notifications@github.com
wrote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a code review, the comment is bound to a line on the diff. So each comment is specific to one line.
I was referring to
var globalspecifically.A name like
var globalNavbarControllerSingletonwould have been better. Long names are not bad, although the usually feel strange.Abbreviations and generic names can be not obvious. Immagine i didn't even know what that line was related to. That may happen to someone editing your code after you.. in order to understand what is
var globalthere for he/she should have read and understood all the rest of the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... Ya I didn't like using that particular scope. But if you look at why
it is there either I am missing something important or it is just not
possible another way.
On Sep 7, 2016 3:08 PM, "Maurizio Casimirri" notifications@github.com
wrote: