Improvements to an Angular ScrollSpy module

Things that I'm not sure about:

• Whether this works in all use cases - alongside routing and within templates etc

• 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?

• Should I implement a scroll throttle?

• Logic assumes elements are written in order in the HTML (last directive hit will be overwritting the current activeSpy value)

• 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 )

• 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.

    New York
    London
    Sydney

```
'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 = el

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

StackExchange Code Review Q#43064, answer score: 6