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();