Added changes in reviews.js to support the new dirtyStateChanged signal in djblets

Review Request #3033 — Created March 31, 2012 and discarded

Information

Review Board

Reviewers

Added changes in reviews.js to support the new dirtyStateChanged signal in djblets
Tested in Chrome 17.0.963.79 and Firefox 11.0
Description From Last Updated

Since this function appears multiple times, maybe its better to take it out as a separate function. Also, it might …

ME medanat

This should be above all usage of the function. Also, "isDirty".

chipx86chipx86

I'm not a fan of this type of usage. Please use if statements instead.

chipx86chipx86
JI
ME
  1. I see this function being bind to many elements, are you sure you need all of them? Are there any overlaps in your even listeners?
    
    Yazan Medanat
    1. yes..I think we need all of them. The new dirtyStateChanged signal is supposed to replace BeginEdit, cancel and complete signals.
      dirtyStateChanged would overlap a bit with cancel and complete, but Christian is fine about it...I think
    2. My concern is an overlap will trigger gEditCount++ twice, could this be an issue?
    3. It wouldn't be because I removed gEditCount++/gEditCount-- in the handler of BeginEdit/cancel/complete, in fact, I removed the handler of BeginEdit completely
      
  2. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
     
    Since this function appears multiple times, maybe its better to take it out as a separate function.
    
    Also, it might be cleaner to have this as:
    
    is_dirty ? gEditCount++ : gEditCount--;
    1. Hmm...good call...I agree with changing it to a one-liner...and make a function for it
  3. 
      
JI
chipx86
  1. 
      
  2. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
    This should be above all usage of the function.
    
    Also, "isDirty".
  3. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
    I'm not a fan of this type of usage. Please use if statements instead.
  4. 
      
JI
JI
Review request changed

Status: Discarded

Loading...