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

jQueryTools plugin custom effect for caching ctabs

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

Problem

This is a custom effect for jQuery tools that enable caching for ajaxed tabs.
Please advice on anything that I can improve. Note that I'm not sure where to put the loading indicator, so, at least have a look at that.

$.tools.tabs.addEffect("ajax_cache", function(tabIndex, done) {
    //check if content already loaded
    //if yes, display it, and hide the rest
    //if no, send ajax request

    var panes_cont = this.getPanes().eq(0);
    var dest_pane = panes_cont.find("#tabindex_"+tabIndex);
    if(dest_pane.length > 0){
        panes_cont.children().hide();
        dest_pane.show();
    } else {
        panes_cont.children().hide();
        panes_cont.append("");

        var new_pane = $('').attr('id', 'tabindex_'+tabIndex).load(this.getTabs().eq(tabIndex).attr("href"),
        function(){
            panes_cont.find("#tab_loading").remove();
            panes_cont.append(new_pane.show());
        });

    }

    done.call();
});

Solution

From a once over :

  • Whether the content is loaded or not, you call panes_cont.children().hide();, you might as well centralize that 1 call



  • There is too much going on in the creation of new_pane, it ought to be split up.



  • lowerCamelCasing is good for you, also write out things. cont keeps reminding me of continue whereas you probably mean content



  • I prefer the line of comment on top of the function



  • There is no need to check length > 0, you can simply check for length



  • As Bill Barry pointed out, caching the loader image is more efficient



  • Most recent js standards suggest to put strings in single quotes, whichever way you go, you should be consistent for easy reading.



All the above together gives this:

//check if pane already loaded, if so, display it, otherwise send ajax
$.tools.tabs.addEffect("ajax_cache", function(tabIndex, done)
{
  var allPanes    = this.getPanes().eq(0).hide(),
      targetPane  = allPanes.find("#tabindex_"+tabIndex);

  if(targetPane.length)
  {
    targetPane.show();
  } 
  else 
  {
    var loaderImage = allPanes.append(''),
        newPane = $('').attr('id', 'tabindex_'+tabIndex),
        URL = this.getTabs().eq(tabIndex).attr('href');

    newPane.load(URL, function(){
      loaderImage.remove();
      allPanes.append(newPane.show());
    });
  }
  done.call();
});

Code Snippets

//check if pane already loaded, if so, display it, otherwise send ajax
$.tools.tabs.addEffect("ajax_cache", function(tabIndex, done)
{
  var allPanes    = this.getPanes().eq(0).hide(),
      targetPane  = allPanes.find("#tabindex_"+tabIndex);

  if(targetPane.length)
  {
    targetPane.show();
  } 
  else 
  {
    var loaderImage = allPanes.append('<img id="tab_loading" src="/graphics/loading.gif"/>'),
        newPane = $('<div>').attr('id', 'tabindex_'+tabIndex),
        URL = this.getTabs().eq(tabIndex).attr('href');

    newPane.load(URL, function(){
      loaderImage.remove();
      allPanes.append(newPane.show());
    });
  }
  done.call();
});

Context

StackExchange Code Review Q#11467, answer score: 3

Revisions (0)

No revisions yet.