Improvements to an Angular ScrollSpy module

Question

Things that I'm not sure about:

1. Whether this works in all use cases - alongside routing and within templates etc
2. Am I polluting the scope with all these variables? This seems to be the easiest way to do unit tests - or should I be passing the vars into the function? Some ( documentHeight, USerScrolledTop, userScrolledBottom) are also generic to all - should they be contained / updated separately?
3. Should I implement a scroll throttle?
4. Logic assumes elements are written in order in the HTML (last directive hit will be overwritting the current activeSpy value)
5. Is the broadcast/listen logic satisfactory, or should I specifically broadcast the id rather than the generic spied, removing the need for if( scope.scrollspyListen === args.activeSpy )
6. Things I don't know I should be unsure about

Love to get some feedback on this module. Feel free to fork and whatnot over on GitHub as well. Demo.

---

<nav>
 <span class="item" data-scrollspy-listen="newyork">New York</span>
 <span class="item" data-scrollspy-listen="london">London</span>
 <span class="item" data-scrollspy-listen="sydney">Sydney</span>
</nav>
<section id="newyork" data-scrollspy-broadcast></section>
<section id="london" data-scrollspy-broadcast></section>
<section id="sydney" data-scrollspy-broadcast></section>

---

'use strict';
angular.module( 'ngScrollSpy', [] )
 .directive( 'scrollspyBroadcast', [ '$rootScope', function( $rootScope ) {
 return {
 restrict: 'A',
 scope: {},
 link: function( scope, element, attrs ) {
 scope.activate = function() {
 scope.documentHeight = Math.max( document.body.scrollHeight, document.body.offsetHeight, document.documentElement.clientHeight, document.documentElement.scrollHeight, document.documentElement.offsetHeight );
 //distance down the page the top of the window is currently at
 scope.userScrolledTop = ( window.pageYOffset !== undefined ) ? window.pageYOffset : ( document.documentElement || document.body.parentNode || document.body ).scrollTop;
 //distance down the page the bottom of the window is currently at
 scope.userScrolledBottom = scope.userScrolledTop + window.innerHeight;
 scope.elementOffsetTop = element[0].offsetTop;
 scope.elementOffsetBottom = scope.elementOffsetTop + Math.max( element[0].scrollHeight, element[0].offsetHeight );
 scope.triggerOffset = 150;
 //determine if element needs to be triggered by the top or bottom of the window
 if( ( scope.elementOffsetTop - scope.triggerOffset ) < ( scope.documentHeight - window.innerHeight ) ) {
 if( scope.elementOffsetTop <= ( scope.userScrolledTop + scope.triggerOffset ) ) {
 $rootScope.$broadcast( 'spied', {
 'activeSpy': attrs.id
 });
 }
 } else {
 if( scope.userScrolledBottom > ( scope.elementOffsetBottom - scope.triggerOffset ) ) {
 $rootScope.$broadcast( 'spied', {
 'activeSpy': attrs.id
 });
 }
 }
 };
 angular.element( document ).ready( function() {
 scope.activate();
 });
 angular.element( window ).bind( 'scroll', function() {
 scope.activate();
 });
 }
 }
 }])
 .directive( 'scrollspyListen', [ '$rootScope', function( $rootScope ) {
 return {
 restrict: 'A',
 scope: {
 scrollspyListen: '@',
 enabled: '@'
 },
 replace: true,
 transclude: true,
 template: function( element, attrs ) {
 var tag = element[0].nodeName;
 return '<'+tag+' data-ng-transclude data-ng-class="{active: enabled}"></'+tag+'>';
 },
 link: function( scope, element, attrs ) {
 $rootScope.$on('spied', function(event, args){
 scope.enabled = false;
 if( scope.scrollspyListen === args.activeSpy ) {
 scope.enabled = true;
 }
 if( !scope.$$phase ) scope.$digest();
 });
 }
 }
 }]);

Answer 1

Nice work!
I presume the complicated way of getting the right element measurements is to cater for many browsers, won't comment on that.

• I would add some comments on the nature of the 'magic number' scope.triggerOffset = 150; or, better, encapsulate it away as .constant or offer as configurable option, to make your directive more re-usable.

• I find the use of ng-class directive with transclusion inside another directive a bit too complicated. If I understand it right, you don't really need to change the DOM, so don't need a template. All you do is assigning a class, which can be done directly inside your link function.

• This is actually an Anti-Pattern, see here:

  if( !scope.$$phase ) scope.$digest();

• konijn: Thanks, there are many more Angular submissions you can review ;)
• Patrick: Thanks for the points, Haven't had the time of late to push some changes I've been meaning to implement. I'll have to revisit !scope.$$phase and see what was going on there. Hopefully I'll get to posting a newer version in the next few days. I would be interested in whether codereview.stackexchange.com/q/42254/36695 is a good config implementation - it was what I was planning on replicating for this module as well...