Clean up some of the datagrid code.

Review Request #4173 — Created May 26, 2013 and submitted

Information

Djblets
master

Reviewers

Clean up some of the datagrid code.

This is a minor cleanup that reduces all the "this" confusion by
assigning real variables to things as quickly as possible, and reduces
all the "var" statements everywhere. It also adds the "$" prefix to
jQuery-wrapped elements.
Tested reordering columns.
Tested adding/removing columns.
Tested sorting columns.
Description From Last Updated

I'm kind of confused about "this" here. Does .each bind this to the elements? Also, we don't use 'i' so …

daviddavid

Isn't this the same as $grid?

daviddavid

We don't use 'i' in here. Also, same question about this inside $(selector).each()

daviddavid

The second argument to the each iterator function is the element itself, which would simplify this a lot.

daviddavid
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        djblets/media/js/datagrid.js
    
    
  2. 
      
david
  1. 
      
  2. djblets/media/js/datagrid.js (Diff revision 1)
     
     
     
    Show all issues
    I'm kind of confused about "this" here. Does .each bind this to the elements?
    
    Also, we don't use 'i' so might as well not include it in the parameters.
    1. jQuery does weird things with "this" everywhere. Any time you loop over a set of elements, "this" refers to the current element.
      
      So in this case above, "this" means the current "col".
      
      Fixing i.
  3. djblets/media/js/datagrid.js (Diff revision 1)
     
     
    Show all issues
    Isn't this the same as $grid?
    1. And then jQuery-UI sets "this" to be the thing being dragged.
  4. djblets/media/js/datagrid.js (Diff revision 1)
     
     
    Show all issues
    We don't use 'i' in here. Also, same question about this inside $(selector).each()
  5. djblets/media/js/datagrid.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    The second argument to the each iterator function is the element itself, which would simplify this a lot.
    1. Ah. Yes, will update for that.
  6. djblets/media/js/datagrid.js (Diff revision 1)
     
     
     
     
    One cute syntactic trick I noticed in the pdf.js source was defining the length inline in here:
    
    for (i = 0, rowsLen = table.rows.length; i < rowsLen; i++) {
  7. 
      
chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        djblets/media/js/datagrid.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed