jQuery highlightNavigation plugin

Question

I have written my first jQuery plugin using one of the design patterns suggested on jQuery's site: http://docs.jquery.com/Plugins/Authoring.

As listed below in the description comment, it's purpose is to provide keyboard and mouse navigation and highlighting for the children of elements like tables, ul's, ol's, and nav's. Believe it or not, I could not find a good, current one that fits my needs...

I want to publish it, but I also want to ensure that's it's good enough to throw out there before doing so, so I am asking for a review.

There are some specific things I have questions about, so I will list them all out.

• My main concern is that in my functions, I wasn't quite clear concerning context, possibly. In order to reference vars declared in the init, one way I found that I could do it was to throw away the var statement, but I know it's best practice to always have them. Otherwise, I can reference the init var's by referring to $.fn.highlightNavigation, which is what is shown below. Seems much better, but I'm just not sure that's good practice. I feel silly about this one..
• The .js file is loaded with comments. I did this in order to make everything to clear to anyone who is working with it. Is there a best practice concerning how much you comment?
• I didn't want to style everything for the author thinking it may make it difficult when they are working with already styled elements and they wish to keep those styles. So, I just kept it to a bare minimum. Thought on this?
• Are there any other things you can see that needs some clarification, fixing, or improvement?

/**
 * Name: highlight-navigation jQuery plugin
 * Author: Stephanie Fischer
 * Description: Provides keyboard and mouse navigation and highlighting for elements with items, such as rows belonging to tables, li's belonging to ul's and ol's, and a's belonging to nav's. Currently only the elements listed are supported, but please extend if you see a need for another element. Also, at this time, it will only work for one instance.
 */

;(function($){
  methods = {
   init: function(options){
    var self = this,
        $self = $(this);
    $.fn.highlightNavigation.self = $self;
    self.wrapper = $("<div class=\"highlight-navigation-container\"/>");
    $.fn.highlightNavigation.self
     .after(self.wrapper)
      .detach()
       .appendTo(self.wrapper);
    $.fn.highlightNavigation.elemObjTag = $.fn.highlightNavigation.self.prop("tagName");
    settings = $.extend({}, $.fn.highlightNavigation.defaults, options);
    this.selectedItem = {};
    $(document).on("keydown", $.proxy(methods.keyPress, self)); //bind keydown to keyPress function.
    $(document).on("click", $.proxy(methods.itemClick, self)); //bind click to itemClick function.
    $(document).on("touchstart", $.proxy(methods.itemClick, self)); //bind touchstart (mobile) to itemClick function.
    methods.selectFirst();
  },
  selectFirst: function(){
   var $firstItem = {},
       $allItems = methods.getAllItems();
   if($allItems){
    $firstItem = $allItems.eq(0);
    $firstItem.addClass("selected");
    $.fn.highlightNavigation.selectedItem = $firstItem; //Set public selectedItem var to first item.
    settings.onSelect(); //onSelect callback.
    settings.selectFirst(); //selectFirst callback.
   }
  },
  selectNext: function(direction){ //Apply highlighting for item we're navigating to.
   var $selectedItem = {},
       $currentselectedItem = methods.getSelectedItem();
   if(direction == -1 && $currentselectedItem.prev().prop("tagName") != undefined){ //Go to previous item, if it exists.
    $selectedItem = $currentselectedItem.prev();
    $selectedItem.addClass("selected");
   }
   else if(direction == 1 && $currentselectedItem.next().prop("tagName") != undefined){ //Go to next item, if it exists.
    $selectedItem = $currentselectedItem.next();
    $selectedItem.addClass("selected");
   }
   if(!$.isEmptyObject($selectedItem)){ //New item was selected
    $currentselectedItem.removeClass("selected"); //Remove selected class from current item.
    $.fn.highlightNavigation.selectedItem = $selectedItem; //Set selectedItem.
   }
   settings.onSelect(); //onSelect callback.
   settings.selectNext(); //selectNext callback.
  },
  keyPress: function(e){ //Handle key press.
   var keyCode = "",
       direction = "";
   e.preventDefault();
   if(window.event){
    keyCode = window.event.keyCode;
   }
   else if(e){
    keyCode = e.which;
   }
   direction = (keyCode == settings.navPrevItemKey)?-1:(keyCode == settings.navNextItemKey)?1:0; //Previous or Next key was pressed, select applicable item.
   if(direction != 0){
    methods.selectNext(direction); //Call selectNext on item we're navigating to.
   }
   if(keyCode == settings.navActionKey){ //Action key (ie: Enter) was pressed, take applicable action defined in callback.
    settings.actionKeyPress();
   }
   this.keyCode = keyCode; //Set public keyCode.
   settings.keyPress(); //keyPress callback
  },
  itemClick: function(e, data){ //Perform item click.
   var self = this,
       evt = (e)?e:event,
       itemClicked = (evt.srcElement)?evt.srcElement:evt.target,
       $allItems = methods.getAllItems(),
       $itemClicked = {},
       $selectedItem = methods.getSelectedItem();
   e.preventDefault();
   if($.fn.highlightNavigation.elemObjTag == "TABLE"){
    $itemClicked = $(itemClicked).parent()
   }
   else{
    $itemClicked = $(itemClicked);
   }
   if($itemClicked.parent().prop("tagName") != "TH"){ //If table, only apply selection for the rows that are not in the table header.
    $allItems.removeClass("selected"); //Remove selected class from all other items.
    $itemClicked.addClass("selected"); //Apply selected class to item just clicked.
    $.fn.highlightNavigation.selectedItem = $itemClicked; //Set public selectedItem.
    settings.itemClick(); //itemClick callback
   }
  },
  getAllItems: function(){ //Return all items.
   if($.fn.highlightNavigation.elemObjTag == "TABLE"){ //note: make into case statement
    return $.fn.highlightNavigation.self.find("tbody tr");
   }
   else if($.fn.highlightNavigation.elemObjTag == "UL" || $.fn.highlightNavigation.elemObjTag == "OL"){
    return $.fn.highlightNavigation.self.find("li");
   }
   else if($.fn.highlightNavigation.elemObjTag == "NAV"){
    return $.fn.highlightNavigation.self.find("a");
   }
   else{
    return false;
   }
  },
  getSelectedItem: function(){ //Public method to return selected item.
   return $.fn.highlightNavigation.selectedItem;
  },
  selectItem: function(index){ //Public method to return selected item.
   var allItems = methods.getAllItems(),
       $selectedItem = $(allItems.get(index));
   allItems.removeClass("selected"); //Remove selected class from all other items.
   $selectedItem.addClass("selected"); //Apply selected class to newly selected item.
   $.fn.highlightNavigation.selectedItem = $selectedItem; //Set selectedItem.
   settings.onSelect(); //onSelect callback.
   return $selectedItem; //return newly selected item.
  },
  getKeyCode: function(){ //Return keycode.
   return $.fn.highlightNavigation.keyCode;
  },
  destroy: function(){ //Undo everything
   var self = this;
   methods.getSelectedItem().removeClass("selected"); //Remove selected class
   $(document).off("keydown", $.proxy(methods.keyPress, self)); //unbind keydown from keyPress function.
   $(document).off("click", $.proxy(methods.itemClick, self)); //unbind click from itemClick function.
   $(document).off("touchstart", $.proxy(methods.itemClick, self)); //unbind touchstart (mobile) from itemClick function.
   $.fn.highlightNavigation.self.unwrap(); //Remove container element
  }
 };
 $.fn.highlightNavigation = function(method){
  $.fn.highlightNavigation.defaults = {
   navPrevItemKey: 38, //Up arrow
   navNextItemKey: 40, //Down arrow
   navActionKey: 13, //Enter
   selectFirst: function(){},
   selectNext: function(){},
   onSelect: function(){},
   keyPress: function(){},
   itemClick: function(){},
   getAllItems: function(){},
   selectItem: function(){},
   getKeyCode: function(){},
  };
  if(methods[method]){
   return methods[method].apply(this, Array.prototype.slice.call(arguments, 1));
  }
  else if(typeof method === "object" || !method){
   return methods.init.apply(this, arguments);
  }
  else{
   $.error("Method " + method + " does not exist in highlight-navigation");
  }
 };
})(jQuery);

Here's the simple css file that will be bundled with it:

.highlight-navigation-container .selected{
 background: #dde3ff;
}

Here is an example of it's usage with just some basic callbacks to open a new page based on an ID belonging to the table row (marked up in the HTML, not the plugin):

  $(".table")
   .highlightNavigation({
    actionKeyPress: function(event, data){ //When action key (enter) is pressed, or row is clicked, go to applicable record.
     var $this = $(this),
         $selectedRow = $this.highlightNavigation("getSelectedItem"),
         keyCode = $this.highlightNavigation("getKeyCode");
     location.href = "users.html#" + $selectedRow.data("id");
    },
    itemClick: function(event, data){ //When action key (enter) is pressed, or row is clicked, go to applicable record.
     var $this = $(this),
         $selectedRow = $this.highlightNavigation("getSelectedItem"),
         $selectedRowID = $selectedRow.data("id");
     if($selectedRowID != undefined){
      location.href = "users.html#" + $selectedRowID;
     }
    }
   });
   selectThree= $(".table").highlightNavigation("selectItem", 3); //select and highlight item 4.

Answer 1

A late review:

• Yes, declaring variables without var is criminal, your approach will do. Do read up on the revealing module pattern to avoid the $.fn.highlightNavigation approach.

• Comments are hard actually, you have a great set here :

  $(document).on("keydown", $.proxy(methods.keyPress, self)); //bind keydown to keyPress function.
  $(document).on("click", $.proxy(methods.itemClick, self)); //bind click to itemClick function.
  $(document).on("touchstart", $.proxy(methods.itemClick, self)); //bind touchstart (mobile) to itemClick function.
  

  • I can see you are connecting keydown to keypress, and the comment calls out that you are doing something unusual. Unfortunately you decided not to comment why you did this
  • click to itemClick is so obvious that the comment is a waste of reading time
  • The third comment is good, because you indicate what you are doing and why (mobile)
• Your sparse usage of styles is a good idea

Other than that, from a once over:

• This is too verbose:

  $selectedItem = $currentselectedItem.prev();
  $selectedItem.addClass("selected");
  

  I would rather see

  $currentselectedItem.prev().addClass("selected");
  

  With or without a line of comment

• The one line indentation for the function body is consistent, but a tad too hard to read and unorthodox

• I see use ternaries, but perhaps not enough, this

  if(window.event){
   keyCode = window.event.keyCode;
  }
  else if(e){
   keyCode = e.which;
  }
  

  could be

  keyCode = window.event ? window.event.keyCode : e.which;
  

• Even if you are not going to use the revealing module pattern, you could use 'sugar' variables to make things less repetitive, so that this

  getAllItems: function(){ //Return all items.
   if($.fn.highlightNavigation.elemObjTag == "TABLE"){ //note: make into case statement
    return $.fn.highlightNavigation.self.find("tbody tr");
   }
   else if($.fn.highlightNavigation.elemObjTag == "UL" || $.fn.highlightNavigation.elemObjTag == "OL"){
    return $.fn.highlightNavigation.self.find("li");
   }
   else if($.fn.highlightNavigation.elemObjTag == "NAV"){
    return $.fn.highlightNavigation.self.find("a");
   }
   else{
    return false;
   }
  },
  

  becomes

  getAllItems: function(){ 
   var tag = $.fn.highlightNavigation.elemObjTag,
       findFunction = $.fn.highlightNavigation.self.find;
   if( tag == "TABLE"){ //note: make into case statement
    return findFunction("tbody tr");
   }
   if(tag  == "UL" || tag  == "OL"){
    returnfindFunction("li");
   }
   if(tag == "NAV"){
    return findFunction("a");
   }
   return false;
  },              
  

  note that else if is not required if the previous if block has a return statement. It makes things more readable.

• In this code block

  itemClick: function(e, data){ //Perform item click.
     var self = this,
     evt = (e)?e:event,
     itemClicked = (evt.srcElement)?evt.srcElement:evt.target,
     $allItems = methods.getAllItems(),
     $itemClicked = {},
     $selectedItem = methods.getSelectedItem();
  

  You are not using data, self or $selectedItem. You can use a tool like JsHint to find such inconsistencies.