3
\$\begingroup\$

This code adds the toggle functionality for mobile devices. So, for width lower than 990px, the panel collapses, and the user can toggle to open / close it.

angular.module('app')
.directive('ngContentToggle', function($window) {

    return {
        link: function(scope, $element){
            var w = angular.element($window, $element);

            scope.toggleClass = function(){
                if($window.outerWidth < 990) {
                    $element.addClass('slide-content');
                }
                else {
                    $element.removeClass('slide-content');
                }
            };

            w.bind('resize load', function(){
               scope.toggleClass();
            });

            $element.bind('click', function(){
                if($element.hasClass('slide-content')){
                    if(($element).next().height > 0) {
                        $element.next().toggleClass('slide-content-open');
                    }
                    else {
                        $element.next().toggleClass('slide-content-open');
                    }
                }
            });
        }
    };
});
\$\endgroup\$
2
\$\begingroup\$

This won't be a very in-depth review since I don't know angular.js, and I will focus only on some superficial issues.

  1. You have "magic" classes.

    These should be declared at the top, to avoid any kind of problem.

    An example:

    var classes = {
        open: 'slide-content-open',
        slide: 'slide-content'
    };
    

    This could help a lot with any future class change.

  2. You repeat $element.next() 3 times.

    This shows a little of misuse of the library, and you should be careful with this since it kills performance.

    Try to save it into a variable or chain it's methods, if possible.

  3. You have a "magic" number there!

    Imagine that now you want to change the width that the class it removed from 990 to, say, 1020. You can argue and say "I just edit it manually, duh!". Now imagine you have 300 lines of code and you don't know where it is and it's previous value.

    I recommend storing that value in a variable by itself.

  4. You can eliminate a lot of bloat by reducing duplicated conditions.

    You have the following code:

    if($element.hasClass('slide-content')){
        if(($element).next().height > 0) {
            $element.next().toggleClass('slide-content-open');
        }
        else {
            $element.next().toggleClass('slide-content-open');
        }
    }
    

    So, no matter what you do, it will toggle the class? Why have this whole condition?

    Eliminating most of the code:

    if($element.hasClass('slide-content')){
        $element.next().toggleClass('slide-content-open');
    }
    

    For some reason, this code seems to look a little like jQuery to me. If you have jQuery, this fits in 1 single line:

    $element.filter('.slide-content').next().toggleClass('slide-content-open');
    
  5. You don't use the variable w anywhere, besides in 1 method a few lines below.

    Instead of all that, you can chain the methods and spare a dangling reference later on.

    Something like this:

    angular
        .element($window, $element)
        .bind('resize load', function(){
           scope.toggleClass();
        });
    

For the final code:

  • Without jQuery:

    angular
        .module('app')
        .directive('ngContentToggle', function($window) {
    
            var classes = {
                open: 'slide-content-open',
                slide: 'slide-content'
            };
    
            var minWidth = 990;
    
            return {
                link: function(scope, $element) {
                    angular
                        .element($window, $element)
                        .bind('resize load', function(){
                           scope.toggleClass();
                        });
    
                    scope.toggleClass = function(){
                        if($window.outerWidth < minWidth) {
                            $element.addClass(classes.slide);
                        }
                        else {
                            $element.removeClass(classes.slide);
                        }
                    };
                    $element.bind('click', function(){
                        if($element.hasClass(classes.slide)){
                            $element.next().toggleClass(classes.open);
                        }
                    });
                }
            };
        });
    
  • With jQuery:

    angular
        .module('app')
        .directive('ngContentToggle', function($window) {
    
            var classes = {
                open: 'slide-content-open',
                slide: 'slide-content'
            };
    
            var minWidth = 990;
    
            return {
                link: function(scope, $element) {
                    angular
                        .element($window, $element)
                        .bind('resize load', function(){
                           scope.toggleClass();
                        });
    
                    scope.toggleClass = function(){
                        if($window.outerWidth < minWidth) {
                            $element.addClass(classes.slide);
                        }
                        else {
                            $element.removeClass(classes.slide);
                        }
                    };
                    $element.bind('click', function(){
                        $element.filter('.' + classes.slide).next().toggleClass(classes.open);
                    });
                }
            };
        });
    
\$\endgroup\$
  • \$\begingroup\$ thank you so much for your time. I have a jquery background and try to adapt to a different way of thinking your comments help me a lot, thanks! \$\endgroup\$ – theoni.n Aug 4 '15 at 12:03
  • \$\begingroup\$ @user1622009 I'm glad I could help you. If you see anything else I can do or if there is something I wasn't accurate about, you can always drop here a comment and I'll do my best to answer as fast as possible \$\endgroup\$ – Ismael Miguel Aug 4 '15 at 12:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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