-
-
-
-
-
djblets/media/js/jquery.gravy.js (Diff revision 1) You can't have both, and it's a boolean, so just do a standard if/else.
-
-
-
djblets/media/js/jquery.gravy.js (Diff revision 1) 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.
-
-
djblets/media/js/jquery.gravy.js (Diff revision 1) It doesn't make sense to go into edit mode when disabled.
change inlineEditor() so it can be enabled/disabled
Review Request #3560 — Created Nov. 25, 2012 and submitted
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. |
chipx86 | |
Seems redundant with this.options.enable. |
chipx86 | |
Only one blank line. |
chipx86 | |
You can't have both, and it's a boolean, so just do a standard if/else. |
chipx86 | |
No blank line. |
chipx86 | |
Add a blank line. |
chipx86 | |
I'm not wild about different things controlling this setting. I'm also not sure we should be able to be in … |
chipx86 | |
Blank line. |
chipx86 | |
It doesn't make sense to go into edit mode when disabled. |
chipx86 | |
So, I think Christian was saying before that if we set this to be enabled instead of enable, we can … |
mike_conley | |
Let's not add a trailing comma if we don't need it. |
mike_conley | |
Remove this newline. |
mike_conley | |
So we won't need this anymore, since this.options.enabled will default to true. |
mike_conley | |
This function needs documentation. |
mike_conley | |
Use this.options.enabled instead. |
mike_conley | |
space after if |
mike_conley | |
Set this.options.enabled instead. |
mike_conley | |
This function needs documentation. |
mike_conley | |
use this.options.enabled |
mike_conley | |
Set this.options.enabled |
mike_conley | |
No need for this newline. |
mike_conley | |
Same line |
DD ddruska | |
Blank line. |
chipx86 | |
Just a nit - these comments don't need to be broken up so short. 80 chars is our limit, so … |
mike_conley | |
It's true, that's what this does, and that's good info to have, but the summary of this function should be … |
chipx86 | |
Here too. |
chipx86 | |
Here too. |
chipx86 | |
Same as above. |
mike_conley | |
Missing period. |
chipx86 | |
Here too. |
chipx86 | |
Be careful with your spacing. There should have been a space before "else". Consistency matters. |
chipx86 | |
And here. |
chipx86 |
-
-
djblets/media/js/jquery.gravy.js (Diff revision 2) 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.
-
djblets/media/js/jquery.gravy.js (Diff revision 2) Let's not add a trailing comma if we don't need it.
-
-
djblets/media/js/jquery.gravy.js (Diff revision 2) So we won't need this anymore, since this.options.enabled will default to true.
-
-
-
-
-
-
-
-
Change Summary:
Again modified logic of enable/disable functions. TODO: document enable and disable functions.
Diff: |
Revision 3 (+38 -5) |
---|
Change Summary:
-Inline Documentation for enable and disable functions. - Modified logic of enable and disable functions.
Diff: |
Revision 4 (+35 -4) |
---|
-
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?
-
djblets/media/js/jquery.gravy.js (Diff revision 4) 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.
-
-
-
-
djblets/media/js/jquery.gravy.js (Diff revision 4) 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.
-
-
-
-
Change Summary:
-Addressed style raised issues. -Changed documentation for enable and disable functions. -Included a 'testing done' section. TODO: reproduce issue Mike is running into.
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+35 -4) |
-
-
djblets/media/js/jquery.gravy.js (Diff revision 5) Be careful with your spacing. There should have been a space before "else". Consistency matters.
-