patternjavascriptMinor
jQuery Plugin Review - wrapper for jQuery UI's tabs
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:
I'd love to get some suggestions on how to improve this, as it'll be used widely in the framework.
- 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
-
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:
-
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.