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

jQuery Plugin Review - wrapper for jQuery UI's tabs

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

Problem

This is a jQuery plug-in I've created that relies on jQuery UI's tabs plug-in. It adds some framework specific logic when setting up tabs:

  • Fixes a bug in jQuery that appears when an id/class contains a "."



  • Manages the tab state using a framework specific cookie API (does not use the jQuery cookie plugin)



  • Exposes a couple of options for transition behavior



I'd love to get some suggestions on how to improve this, as it'll be used widely in the framework.

(function ($) {
    $.fn.dnnTabs = function (options) {
        var opts = $.extend({}, $.fn.dnnTabs.defaultOptions, options),
            $wrap = this;

        $(document).ready(function () {
            // Patch for Period in IDs (jQuery bug)
            $.ui.tabs.prototype._sanitizeSelector = function (hash) {
                // we need this because an id may contain a ":" or a '.'
                return hash.replace(/:/g, "\\:").replace(/\./g, "\\\.");
            };

            var id = 'dnnTabs-' + $wrap.selector.replace('#', '').replace('.', '');
            if (opts.selected === -1) {
                var tabCookie = dnn.dom.getCookie(id);
                if (tabCookie) {
                    opts.selected = tabCookie;
                }
                if (opts.selected === -1) {
                    opts.selected = 0;
                }
            }

            $wrap.tabs({
                show: function (event, ui) {
                    dnn.dom.setCookie(id, ui.index, 1, '/', '', false);
                },
                selected: opts.selected,
                fx: {
                    opacity: opts.opacity,
                    duration: opts.duration
                }
            });
        });

        return $wrap;
    };

    $.fn.dnnTabs.defaultOptions = {
        opacity: 'toggle',
        duration: 'fast',
        selected: -1
    };

})(jQuery);

Solution

These are probably more comments, but I'll post them as an "answer", because it's easier.

-
Could you point out a source or an example for the bug with ids/classes containing . and :, because in a quick test they seem to work fine: http://jsfiddle.net/NSXLc/

-
I don't think you should use the document ready event. It should be the plugins user's choice when he wants to apply the tabs.

-
The way you are getting the id for the cookie looks dangerously wrong. You assume the selector is always a simple id or class selector and extending on that you assume, that the selector only matches one element and thus only one one set of tabs are initialized at a time.

It's probably better to do something like this:

$wrap.each(function() { // In case the selector matches multiple elements
  var showEvent = null;
  if (this.id) { // only use cookie if ID is set. TODO: use className as alternative
    var id = 'dnnTabs-' + this.id;
    if (opts.selected === -1) {
      // ...
    }

    showEvent = (function(cookieId) { // closure for cookie id
      return function (event, ui) {
        dnn.dom.setCookie(cookieId, ui.index, 1, '/', '', false);
      }
    })(id);
  }
  $(this).tabs({
    show: showEvent,
    // ...
  });
});

Code Snippets

$wrap.each(function() { // In case the selector matches multiple elements
  var showEvent = null;
  if (this.id) { // only use cookie if ID is set. TODO: use className as alternative
    var id = 'dnnTabs-' + this.id;
    if (opts.selected === -1) {
      // ...
    }

    showEvent = (function(cookieId) { // closure for cookie id
      return function (event, ui) {
        dnn.dom.setCookie(cookieId, ui.index, 1, '/', '', false);
      }
    })(id);
  }
  $(this).tabs({
    show: showEvent,
    // ...
  });
});

Context

StackExchange Code Review Q#2096, answer score: 2

Revisions (0)

No revisions yet.