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

Creating a tab plugin

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

Problem

I am creating a tab plugin. I want to know if there is a better way of doing this or if what I have is good. It works just fine, but there may be some shortcuts or a more optimized way of accomplishing this. I plan to replace the startTab variable with an options set, but I am not quite there yet.

View the current updated version here

Here is what I have so far:

`

Untitled Document

ul.tabNavigation {
list-style: none;
margin: 0;
padding: 0;
}
ul.tabNavigation li {
display: inline;
}
ul.tabNavigation li a {
text-decoration: none;
color:#000;
}
.tabViews {
width: 100%;
border: #8db2e3 solid 1px;
clear:both;
height:18px;
margin-top: -12px;
background: url(css/tabs/images/tabs_Lower_Header_Background.png) repeat-x;
}
.tab {
height: 25px;
font-family:'Trebuchet MS', Arial, Helvetica, sans-serif;
font-size:14px;
float:left;
padding-left: 4px;
position: relative;
top: 1px
}
.tab .right {
float:left;
background:url(css/tabs/images/tab_Selected_Right.png) no-repeat;
height:25px;
width: 8px;
}
.tab .content {
float:left;
background:url(css/tabs/images/tab_Selected_Content.png) repeat-x;
height:25px;
text-align: center;
padding-left: 5px;
padding-right: 5px;
padding-top: 4px;
}
.tab .left {
float:left;
background:url(css/tabs/images/tab_Selected_Left.png) no-repeat;
height:25px;
width: 6px;
}
.tabHover {
height: 25px;
font-family:'Trebuchet MS', Arial, Helvetica, sans-serif;
font-size:14px;
float:left;
padding-left: 4px;
position: relative;
top: 1px;
cursor: pointer;
}
.h1 {
float:left;
height:25px;
width: 14px;
}
.h2 {
float:left;
height:25px;
text-align: center;
padding-left: 7px;
padding-right: 5px;
padding-top: 4px;
}
.h3 {
float:left;
height:25px;
width: 10px;
}
.tabHover:hover .h1 {
background:url(css/tabs/images/tab_Hover_Right.png) no-repeat;

Solution

You could remove tab navigation from html and create it dynamically in plugin so the html will look like this:


  
    Test
    test this
  


you don't need to call

var tabsContainer = $(this);


this is already jQuery object

inside switchClass function you call $(this) 3 times you can call it once and stor the value

var $this = $(this);


you get element from jQuery object and then wrap it again.

$(classBuilder[0]).removeClass('h3');


you could use eq jquery method

classBuilder.eq(0).removeClass('h3');


instead

if(index == selectedIndex){

} 
if(index == currentIndex){

}


use

if(index == selectedIndex){

} else if(index == currentIndex){

}


you don't need to check if index == currentIndex if index == selectedIndex

this code is repeaded twice but with different class names create a function for it (it looks almost the same — if you see something like this always create new function)

$(classBuilder[0]).removeClass('h3');
$(classBuilder[0]).addClass('left');
$(classBuilder[1]).removeClass('h2');
$(classBuilder[1]).addClass('content');
$(classBuilder[2]).removeClass('h1');
$(classBuilder[2]).addClass('right');


and you can chain jquery methods:

classBuilder.eq(0).removeClass('h3').addClass('left');
classBuilder.eq(1).removeClass('h2').addClass('content');
classBuilder.eq(2).removeClass('h1').addClass('right');


you don't need to iterate over $tabs

function switchClass(selectedIndex){ 
   $tabs.eq(selectedIndex)


and I think that this:

var classBuilder = $tabs.eq(selectedIndex).find('div  > div');


will be the same as

var classBuilder = $('div  > div',$(this))


and this:

if(index == currentIndex){
    var tabContainer = $('a > div',$(this))
    tabContainer.removeClass('tab');
    tabContainer.addClass('tabHover');


is the same as

$tabs.eq(currentIndex).find('a > div').removeClass('tab').addClass('tabHover');

and at the end of the script you should return this so it could be chained

$('#rfiTabs').tabs(0).css('background-color', 'red');


you can also do this:

return this.each(function() {
var $this = $(this);
//and create your tabs here
});

so if you call it

$('.myalltabs').tabs(0);


it will create tabs for every element that have .myalltabs class

Update

if you your jquery object have multiple elements (But this is Something Completely Different)

$.fn.tabs = function(n) {

    var = tabcontainer = $('').attr('class', 'mainTabClass')
             .appendTo($('body'));

    var nav = $('').addClass('navigation').appendTo(tabcontainer);
    this.each(function() {
       var $this = $(this);
       var id = $this.attr('id');
       $('').append('').appendTo(nav).children().
          attr('href', '#' + id).html($this.attr('name'));
    }).addClass('tabContent').detach().appendTo(tabcontainer);

    nav.find('li a').click(function() {
      tabcontainer.find('tabContent').removeClass('selected');
      tabcontainer.find($(this).attr('href')).addClass('selected');
    }).eq(n).addClass('selected');
};


in this case you need only

.selected {
   display: block;
}
.mainTabClass .tabContent {
   display: none;
}

  ....
  this is home
  ....
  this is about
  ....


name attribute in case you won't different name than id.

$('#home, #about').tabs(1);


and you will have


  
    Home Page
    About me
  
  this is home
  this is about


I don't know why you have repeated left, right and content for every tab, if you need to use those - create one instance and change it on click (in case you will have 100 tabs you will have 3 DOM elements instead of 300)

Code Snippets

<div id="rfiTabs">
  <div class="tabViews" style="">
    <div id="rfiBasic">Test</div>
    <div id="rfiHome">test this</div>
  </div>
</div>
</body>
</html>
var tabsContainer = $(this);
var $this = $(this);
$(classBuilder[0]).removeClass('h3');
classBuilder.eq(0).removeClass('h3');

Context

StackExchange Code Review Q#908, answer score: 6

Revisions (0)

No revisions yet.