patternjavascriptMinor
Width-based panel collapsion
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:
This could help a lot with any future class change.
-
You repeat
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:
So, no matter what you do, it will toggle the class? Why have this whole condition?
Eliminating most of the code:
For some reason, this code seems to look a little like jQuery to me. If you have jQuery, this fits in 1 single line:
-
You don't use the variable
Instead of all that, you can chain the methods and spare a dangling reference later on.
Something like this:
For the final code:
-
Without jQuery:
-
With jQuery:
-
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.