change inlineEditor() so it can be enabled/disabled

Review Request #3560 — Created Nov. 25, 2012 and submitted

Information

Djblets
master

Reviewers

The purpose of this change is to allow enabling/disabling of the '.editable' fields of a review request.
Note: 
-testing was performed in latest version of Chrome and Firefox.
-testing was performed with input in all of the fields as well as in none of the fields.

Expected behaviour: 
Review request fields should be disabled when the review request is in a non-editable state(submitted/discarded)
Review request fields should be enabled when the review request is in a editable state(draft/public)

* Brand new, not yet published:
discard-review-request button: review request fields cleared and disabled as expected.
discard-review-request link: review request fields cleared and disabled as expected.
Publish: review request fields remain enabled as expected.

* Published and unchanged:
close-discarded-review-request link: review request fields are disabled and set to the upstream review request.
close-submitted-review request link: '.editable' fields are disabled and set to the upstream review request.

* Discarded:
close-discarded-review-request link: '.editable' fields are disabled and set to the upstream review request.

* Reopened after discarded:
review request fields are enabled as expected. 

* Submitted:
close-submitted-review request link: '.editable' fields are disabled and set to the upstream review request.

* Reopened after submitted:
review request fields are enabled and set to the upstream review request. Behaves as expected for requests that were submitted as drafts as well as public.
Description From Last Updated

Should be listed alphabetically.

chipx86chipx86

Seems redundant with this.options.enable.

chipx86chipx86

Only one blank line.

chipx86chipx86

You can't have both, and it's a boolean, so just do a standard if/else.

chipx86chipx86

No blank line.

chipx86chipx86

Add a blank line.

chipx86chipx86

I'm not wild about different things controlling this setting. I'm also not sure we should be able to be in …

chipx86chipx86

Blank line.

chipx86chipx86

It doesn't make sense to go into edit mode when disabled.

chipx86chipx86

So, I think Christian was saying before that if we set this to be enabled instead of enable, we can …

mike_conleymike_conley

Let's not add a trailing comma if we don't need it.

mike_conleymike_conley

Remove this newline.

mike_conleymike_conley

So we won't need this anymore, since this.options.enabled will default to true.

mike_conleymike_conley

This function needs documentation.

mike_conleymike_conley

Use this.options.enabled instead.

mike_conleymike_conley

space after if

mike_conleymike_conley

Set this.options.enabled instead.

mike_conleymike_conley

This function needs documentation.

mike_conleymike_conley

use this.options.enabled

mike_conleymike_conley

Set this.options.enabled

mike_conleymike_conley

No need for this newline.

mike_conleymike_conley

Same line

DD ddruska

Blank line.

chipx86chipx86

Just a nit - these comments don't need to be broken up so short. 80 chars is our limit, so …

mike_conleymike_conley

It's true, that's what this does, and that's good info to have, but the summary of this function should be …

chipx86chipx86

Here too.

chipx86chipx86

Here too.

chipx86chipx86

Same as above.

mike_conleymike_conley

Missing period.

chipx86chipx86

Here too.

chipx86chipx86

Be careful with your spacing. There should have been a space before "else". Consistency matters.

chipx86chipx86

And here.

chipx86chipx86
chipx86
  1. 
      
    1. It's also important to know step-by-step how you tested it. Every state combination with this should be tested.
  2. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    Show all issues
    Should be listed alphabetically.
  3. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    Show all issues
    Seems redundant with this.options.enable.
    1. Here's my reasoning for having both. So options.enable allows to always bind '.editable' elements to the inlineEditor() and enable/disable them depending on the state of the review request. 
      
      And _enabled allows to check whether an '.editable' element is enabled/disabled. So that enable() can return early  if it called on an already enabled element. Same reasoning if disable() is called on an already disabled element. 
  4. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
     
     
    Show all issues
    Only one blank line.
  5. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    You can't have both, and it's a boolean, so just do a standard if/else.
  6. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
     
    Show all issues
    No blank line.
  7. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
    Show all issues
    Add a blank line.
  8. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    Show all issues
    I'm not wild about different things controlling this setting. I'm also not sure we should be able to be in a state where we're disabled and editing.
    1. Jesus - let's have the inlineEditor stop editing before disabling, so that if / when the field is re-enabled, it's in the non-editing state.
  9. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
    Show all issues
    Blank line.
  10. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
    Show all issues
    It doesn't make sense to go into edit mode when disabled.
    1. I am setting it to true so that startEdit() returns early(so the field is no longer editable)
    2. It'd make more sense for startEdit and such to check for whether it's enabled. It's still possible that you can disable an editor and then call startEdit on it, breaking the state.
    3. Jesus:
      
      That's not making a whole lot of sense to me.
      
      Look at: https://github.com/djblets/djblets/blob/master/djblets/media/js/jquery.gravy.js#L512
      
      startEdit simply returns if it's called when this._editing is true (since we don't ever want to start editing again if we're already editing to begin with)...
      
      So while startEdit will certainly return early, I'm really seeing no value in it here.
      
      -Mike
  11. 
      
mike_conley
  1. Any updates to this patch, Jesus?
  2. 
      
JZ
mike_conley
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    So, I think Christian was saying before that if we set this to be enabled instead of enable, we can use this.options.enabled instead of baking in our own this._enabled. 
  3. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    Let's not add a trailing comma if we don't need it.
  4. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    Remove this newline.
  5. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    So we won't need this anymore, since this.options.enabled will default to true.
  6. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    This function needs documentation.
  7. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    Use this.options.enabled instead.
  8. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    space after if
  9. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    Set this.options.enabled instead.
  10. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    This function needs documentation.
  11. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    use this.options.enabled
  12. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    Set this.options.enabled
  13. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    Show all issues
    No need for this newline.
  14. 
      
JZ
JZ
mike_conley
  1. So disabling and enabling an inlineEditor seems to work. Good job!
    
    I am noticing, however, that if I have an inlineEditor, if I edit it, press OK, and then edit it again, the text field is significantly shorter than it was before, and the buttons appear on the next line.
    
    Steps to reproduce:
    
    -Create a new review request
    -Click the pencil next to summary, type something in, press enter
    -Click on the pencil again
    
    I'm experiencing this on Ubuntu 12.04 using Firefox. Are you seeing the same thing?
    1. So I missed an absolutely critical part of the STR - you need to disable and re-enable the inlineEditor for that first field.
      
      Here's some code to do that that you can toss in to the console:
      
      var summary = $(".editable").first();
      summary.inlineEditor("disable");
      summary.inlineEditor("enable");
      
      Now repeat steps above.
    2. Hey i tried reproducing the bug with FF nightly and nothing :/ i'm probably doing something wrong?
      Here are a couple of screenshot of the steps i took. http://imgur.com/a/a5A7F#02gqC
    3. Mike, is this specific with Jesus's patch?
    4. Yep - it only seems to occur after I've manually disabled and then re-enabled the inlineEditor.
      
      If nobody can reproduce, it shouldn't block - we can look into it later. 
  2. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Just a nit - these comments don't need to be broken up so short. 80 chars is our limit, so I think you have some more space here.
  3. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
     
    Show all issues
    Same as above.
  4. 
      
DD
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Same line
  3. 
      
chipx86
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Blank line.
  3. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    It's true, that's what this does, and that's good info to have, but the summary of this function should be "Enables use of the inline editor."
    
    Same sort of comment below.
  4. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Here too.
    1. That is, blank line.
  5. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Here too.
  6. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
    Show all issues
    Missing period.
  7. djblets/media/js/jquery.gravy.js (Diff revision 4)
     
     
     
    Show all issues
    Here too.
  8. 
      
JZ
JZ
chipx86
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 5)
     
     
    Show all issues
    Be careful with your spacing. There should have been a space before "else". Consistency matters.
  3. djblets/media/js/jquery.gravy.js (Diff revision 5)
     
     
    Show all issues
    And here.
  4. 
      
JZ
mike_conley
  1. I'm good with this. Thanks Jesus!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
JZ
Review request changed
Status:
Completed
Change Summary:
Pushed to master (301e1b1)