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

jQuery collapse/expand plugin

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

Problem

This is my first plugin for JavaScript. It originated from a need for a similar feature at work and I decided to make it a bit more generic in case I needed it again. I was just wanting some criticism on my code. I added comments on the defaultOptions to try and give a better picture of what the plugin does. Do I have JavaScript and jQuery standards and best practices down?

```
(function ($) {
"use strict";
// Encase this needs changed due to conflict.
var dataKey = "_collapse",
// Plugin instance class
Collapse = function (options, userEvents) {
this.options = options;
this.userEvents = userEvents;
return this;
};

// Plugin's prototype functions
Collapse.prototype = {
enable: function (container) {
var self = this,
addCollapseButtonToThese = container.find("." + this.options.collapser),
collapseThese = container.find("." + this.options.collapsee);

if (collapseThese.length -1,
events = plugin.userEvents;

if (plugin === undefined) {
console.log("Error: Plugin data not found!");
return -1;
}

if (toggleSuccess) {
// Run user-defined event.
if (typeof events.onToggle === "function") {
events.onToggle(e, this);
} else {
console.log("Error: User-defined event: onToggle is not a function!");
}
} else {
console.log("Error: There was an issue toggling collapse! User-defined event not executed.");
}
},
toggle: function (button) {
var collapse = $(button).data(dataKey),
collapsees = collapse.getCollapsees(button);

button = $(button);
if (button.hasClass(collapse.options.collapsed)) {
// Change the button image.

Solution

-
I noticed that your options force strongly push the user to provide a CSS class. This doesn't make much sense to me; it seems like just having a selector instead would be better. As is, if a user wants to get something by ID, then they have to use some shenanigans like setting userOptions.collapser to "doesnotexist, #target".

If you do allow arbitrary selectors, then you could perhaps rename collapsed to collapsedClass, so as to explicitly state it's a class.

-
In the anonymous function in Collapse.prototype.enable(), you're creating $(this) multiple times. This is not good practice; instead, create it once and store it in a local variable, à la var $this = $(this);.

-
With collapseEach, you seem to be adding the options.uncollapsed class to anything that does not have it yet, and also does not have the options.collapsed class. There's no need to check if it has the options.uncollapsed class; jQuery will not add the class twice.

-
As a matter of fact, you can simplify this:

collapseEach.not("." + self.options.collapsed).addClass(self.options.uncollapsed);


-
console.log should be used carefully. In Internet Explorer, the console object won't exist unless the Developer Tools are open at load time. Thus, this will throw a ReferenceError. Either wrap it in an if (typeof console !== 'undefined'), or define it with var console = console || { log: function(){} };.

-
You've got true / false for success / failure in Collapse.prototype.enable(), but undefined / -1 in Collapse.prototype.onToggle(). This feels inconsistent.

-
Your stop variable in Collapse.prototype.getCollapsees() is redundant. You can write it out of the loop condition and just do if (rows.filter ...) break;.

Code Snippets

collapseEach.not("." + self.options.collapsed).addClass(self.options.uncollapsed);

Context

StackExchange Code Review Q#59467, answer score: 2

Revisions (0)

No revisions yet.