• 
      

    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