3
\$\begingroup\$

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("<img id='tab_loading' src='/graphics/loading.gif' />");

        var new_pane = $('<div>').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();
});
\$\endgroup\$
1
  • \$\begingroup\$ Instead of giving the loading image an id and then traversing for it, you should make it a cached jquery object. \$\endgroup\$
    – Bill Barry
    May 4, 2012 at 22:11

1 Answer 1

3
\$\begingroup\$

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('<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();
});
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.