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

jQuery dropdown plugin

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

Problem

I'm looking for OOP tips or advice.

```
;(function ( $, window, document, undefined ) {

if ( typeof Object.create !== 'function' ) {
Object.create = function (o) {
function F() {}
F.prototype = o;
return new F();
};
}

if (!Function.prototype.bind) { // check if native implementation available
Function.prototype.bind = function(){
var fn = this, args = Array.prototype.slice.call(arguments),
object = args.shift();
return function(){
return fn.apply(object,
args.concat(Array.prototype.slice.call(arguments)));
};
};
}

$.component = function( name, object ) {
$.fn[name] = function( options ) {
return this.each(function() {
if ( !$.data( this, name ) ) {
$.data( this, name, Object.create(object).init(
$.extend(true, {}, $.fn[name].opts, options), this )
);
}
});
};
};

})( jQuery, window , document );

(function ( $, window, document, undefined ) {

var dropdown = {
opts: {},
init: function( options, el ) {
this.$el = $(el).on('click.dropdown',
(function(evt){
this.toggle(evt);
}).bind(this)
);
this.opts = $.extend(true, this.opts, options);
this.isShowing = false;
return this;
},
show: function (evt) {
if(evt) evt.preventDefault(); evt.stopPropagation();
$(this.opts.header).addClass('show-mobile-nav');
this.isShowing = true;
this.bindClose();
},
hide: function (evt) {
if(evt) evt.preventDefault();
$(this.opts.header).removeClass('show-mobile-nav');
this.isShowing = false;
return this;
},
bindClose: function () {
$('html').on('click.dropdown',
(function(evt){
var $targ = $(event.target || window.event.srcElement),
targetIsNotDropdown = !$targ.is(this.opts.menu),
targetsNotAChildOfDropdown = $(this.opts.menu).has( $tar

Solution

From a once over:

  • Your code is not consistently indented, especially the first dozen lines need some TLC



  • $.component = function( name, object ) { needs commenting, it is not clear what this is trying to achieve.



  • evt is an unfortunate parameter name, I would go for the classic e or the readable event



  • $targ is needlessly short, just use $target



  • You could probably use helper functions for if(evt) evt.preventDefault(); and if(evt) evt.preventDefault(); evt.stopPropagation();



  • Note that if(evt) evt.preventDefault(); evt.stopPropagation(); is exactly the kind of bug that makes people enforce {} around if blocks!



  • toggle() could use a ternary condition with !this.isShowing



  • 'show-mobile-nav' should probably not be hardcoded since you use it more than once, instead you should use a properly named variable



All in all, some pretty cool code.

Context

StackExchange Code Review Q#25697, answer score: 2

Revisions (0)

No revisions yet.