Prevent duplicate infobox fetches

Review Request #8568 - Created Dec. 12, 2016 and submitted

Barret Rennie
Review Board
release-3.0.x
009ec7b...
reviewboard

Previously, datagrids would attach additional event handler for
infoboxes on top of the initial event handler. This would result in two
fetches for all infoboxes in the datagrid until the first refresh. Now
we set an attribute on elements which can have infoboxes attached them
when the infobox handler has been added so that it will not be added
again.

Verified infoboxes in datagrids are no longer requested twice.

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/common.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/common.es6.js
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. reviewboard/static/rb/js/common.es6.js (Diff revision 1)
     
     

    Can we make this data-has-${id}-infobox? As-is it'll make attributes like data-has-user which isn't immediately obvious.

    1. No it will make data-has-user_infobox etc.

  3. reviewboard/static/rb/js/common.es6.js (Diff revision 1)
     
     

    Early returns increase complexity. Given that the code that would be skipped by it is only three statements, how about we instead do:

    if (!$target.attr(infoboxAttr)) {
        const view = new RB.InfoboxView({
            $target: $target,
            el: $el,
        });
    
        view.render();
        $target.attr(infoboxAttr, true);
    }
    
  4. reviewboard/static/rb/js/common.es6.js (Diff revision 1)
     
     

    This needs to be $target now, because fat-arrow functions have lexical this binding.

    1. Good catch; totally meant to do this.

  5. 
      
Barret Rennie
Review Bot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/common.es6.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/common.es6.js
    
    
  2. 
      
David Trowbridge
  1. Ship It!
  2. 
      
Christian Hammond
  1. 
      
  2. reviewboard/static/rb/js/common.es6.js (Diff revision 2)
     
     
     
     

    We should use .data() instead for storing this, rather than using an attribute.

    We actually can attach the view as the data. This is what jQuery widgets do. This allows for fetching the view for a given element as well.

    (.data() doesn't simply wrap .attr('data-...'))

  3. 
      
Barret Rennie
Review Bot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/common.es6.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/common.es6.js
    
    
  2. 
      
David Trowbridge
  1. Ship It!
  2. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (d2fdef8)
Loading...