patternjavascriptMinor
jQuery collapse/expand plugin
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
```
(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.
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
If you do allow arbitrary selectors, then you could perhaps rename
-
In the anonymous function in
-
With
-
As a matter of fact, you can simplify this:
-
-
You've got
-
Your
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.