jimrrchen got review request #3000!

confirmation for leaving the page now only appears when the text in inlineEditor is actually changed, also fixed a bug with esc key not registered with inlineEditor

Review Request #3000 — Created March 22, 2012 and submitted

Information

Djblets

Reviewers

Confirmation for leaving the page now only appears when the text in inlineEditor is actually changed
Also fixed a bug with esc key not registered with inlineEditor

Changed the behaviour of the editor so now whenever the dirty state is changed a "dirtyStateChanged" event is triggered
Done in Linux environment with Chrome 17 and Firefox 11
Description From Last Updated

We have a dirty() function already. It seems it could just be updated to not assume dirty if in edit …

chipx86chipx86

This will break things. There's distinct differences in how codes come in from keydown and keypress, and what will go …

chipx86chipx86

All comments must use sentence capitalization and trailing periods. For multi-line comments, use: /* * comment line * ... */

chipx86chipx86

So, what does trigger this now? It feels like this will cause API breakage.

chipx86chipx86

I know the old code wasn't styled right, but all jquery stuff should be like: $(el) .keydown(function(e) { }) .keyup(function(e) …

chipx86chipx86

"resets" Period at the end.

chipx86chipx86

Period at the end.

chipx86chipx86

No space before ":"

chipx86chipx86

I'd prefer "curDirtyState" rather than using "curr".

chipx86chipx86
DD
  1. A "dirty" editor is one that has changes in it that need to be saved.
    Congrats on review request #3000!
    1. Thanks :)
      
      it seems like $(a).inlineEditor("dirty") will invoke the dirty() function and I am thinking if there is a way to just use the _dirty field that I added instead of calling the dirty() function
  2. 
      
JI
chipx86
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    Show all issues
    We have a dirty() function already. It seems it could just be updated to not assume dirty if in edit mode. In fact, I think the rest of the change in here could go away with that function being updated?
    
    iirc, our logic for determining whether to show the confirmation dialog on page leave is doing it based on a query using dirty(). I don't see where this ties in to that?
  3. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    Show all issues
    This will break things. There's distinct differences in how codes come in from keydown and keypress, and what will go to what. That's why we have the distinction between these events today. We can't change that.
  4. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
    Show all issues
    All comments must use sentence capitalization and trailing periods.
    
    For multi-line comments, use:
    
    /*
     * comment line
     * ...
     */
  5. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    Show all issues
    So, what does trigger this now? It feels like this will cause API breakage.
  6. 
      
JI
chipx86
  1. Some minor nits. General stuff looks really good.
  2. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    I know the old code wasn't styled right, but all jquery stuff should be like:
    
    $(el)
        .keydown(function(e) {
        })
        .keyup(function(e) {
        });
  3. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    "resets"
    
    Period at the end.
  4. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    Period at the end.
  5. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    No space before ":"
  6. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    I'd prefer "curDirtyState" rather than using "curr".
  7. 
      
JI
chipx86
  1. Ship It!
  2. 
      
JI
Review request changed
Status:
Completed
Change Summary:
Pushed to master (8256d90)