HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Width-based panel collapsion

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
panelwidthcollapsionbased

Problem

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  0) {
                        $element.next().toggleClass('slide-content-open');
                    }
                    else {
                        $element.next().toggleClass('slide-content-open');
                    }
                }
            });
        }
    };
});

Solution

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.

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

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

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

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


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

Code Snippets

var classes = {
    open: 'slide-content-open',
    slide: 'slide-content'
};
if($element.hasClass('slide-content')){
    if(($element).next().height > 0) {
        $element.next().toggleClass('slide-content-open');
    }
    else {
        $element.next().toggleClass('slide-content-open');
    }
}
if($element.hasClass('slide-content')){
    $element.next().toggleClass('slide-content-open');
}
$element.filter('.slide-content').next().toggleClass('slide-content-open');
angular
    .element($window, $element)
    .bind('resize load', function(){
       scope.toggleClass();
    });

Context

StackExchange Code Review Q#98984, answer score: 2

Revisions (0)

No revisions yet.