3
\$\begingroup\$

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 <= 0) {
 console.log("Error: The only children in the container are target children. There is nothing to collapse!");
 return false;
 }
 // Run the addCollapseButton function for each target.
 $(addCollapseButtonToThese).each(function () { self.options.addCollapseButton($(this)); });
 // Add the uncollapsed class to every targetChild.
 collapseThese.each(function () {
 if (!$(this).hasClass(self.options.collapsed) && !$(this).hasClass(self.options.uncollapsed)) {
 $(this).addClass(self.options.uncollapsed);
 }
 });
 // Enable the click event for the toggle button.
 $(document).on("click", "." + this.options.collapserButton, this.onToggle);
 return true;
 },
 onToggle: function (e) {
 e.preventDefault();
 var plugin = $(this).data(dataKey),
 toggleSuccess = Collapse.prototype.toggle(this) > -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.
 collapse.uncollapseElement(button);
 button.attr("src", collapse.options.images.uncollapsed);
 $(collapsees).each(function () {
 collapse.uncollapseElement(this);
 });
 return 0;
 }
 if (button.hasClass(collapse.options.uncollapsed)) {
 // Change the button image.
 collapse.collapseElement(button);
 button.show(); // Keep the button visible.
 button.attr("src", collapse.options.images.collapsed);
 $(collapsees).each(function () {
 collapse.collapseElement(this);
 });
 return 1;
 }
 return -1;
 },
 getCollapsees: function (button) {
 var row = $(button).closest("." + this.options.collapser),
 rows = (this.options.relation === "sibling") ? $(row).siblings() : (this.options.relation === "child") ? $(row).find("." + this.options.collapsee) : $(row),
 index = rows.index(row),
 collapsees = [],
 i = index + 1,
 stop = false,
 collapsee;
 while (!stop && i < rows.length) {
 collapsee = $(rows[i]);
 stop = rows.filter("." + this.options.collapsee).index(collapsee) < 0;
 if (stop) {
 break;
 }
 collapsees.push(collapsee);
 i += 1;
 }
 return collapsees;
 },
 collapseElement: function (element) {
 $(element).removeClass(this.options.uncollapsed).addClass(this.options.collapsed);
 },
 uncollapseElement: function (element) {
 $(element).removeClass(this.options.collapsed).addClass(this.options.uncollapsed);
 }
 };
 // Public function that enables the plugin.
 $.fn.collapse = function (userOptions, userEvents) {
 var options = $.extend({}, $.fn.collapse.defaultOptions, userOptions || {}),
 events = $.extend({}, $.fn.collapse.defaultUserEvents, userEvents || {}),
 plugin = new Collapse(options, events),
 success = false;
 // Initialize the plugin.
 success = plugin.enable($(this));
 // Save a reference of the plugin object
 // Reason: Allow later access.
 // How: Use the data method to keep a copy of the plugin instance with the element.
 // Where: On the collapserButtons.
 $(this).find("." + options.collapserButton).data('_collapse', plugin);
 if (success) {
 plugin.userEvents.onEnable($(this));
 }
 return this;
 };
 // Defaults
 $.fn.collapse.defaultOptions = {
 collapser: "collapser", // CSS class of the container
 collapsee: "collapsee", // CSS class of the items being hidden
 relation: "child", // relation between the collapser and collapsee
 collapserButton: "collapseButton", // CSS class of the toggle button
 collapsed: "collapsed", // CSS class to define the item is collapsed (hidden).
 uncollapsed: "uncollapsed", // CSS class to define the item is uncollapsed.
 addCollapseButton: function (collapser) { }, // function that adds the button
 images: { // image for the button
 collapsed: "Images/collapsed.png", 
 uncollapsed: "Images/uncollapsed.png"
 }
 };
 $.fn.collapse.defaultUserEvents = {
 onToggle: function (e, button) { },
 onEnable: function (container) { },
 onDisable: function (container) { }
 };
}(jQuery));
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Aug 8, 2014 at 4:47
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • 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;.

answered Aug 8, 2014 at 16:41
\$\endgroup\$
2
  • \$\begingroup\$ ah, I forgot I still had console in there. I was planning on switching it to a throw. Would this be better practice? \$\endgroup\$ Commented Aug 8, 2014 at 17:04
  • \$\begingroup\$ And as for the CSS Class vs selector, I previously had a use for it but removed it, so thank you for pointing out I can use any selector now. \$\endgroup\$ Commented Aug 8, 2014 at 17:21

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.