6
\$\begingroup\$

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

                });

            }
        }
    }]);
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ Thanks, there are many more Angular submissions you can review ;) \$\endgroup\$
    – konijn
    Apr 22, 2014 at 14:02
  • \$\begingroup\$ 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... \$\endgroup\$
    – Patrick
    Apr 22, 2014 at 15:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.