Summary

change inlineEditor() so it can be enabled/disabled

Review Request #3560 — Created Nov. 25, 2012 and submitted Dec. 4, 2012, 11:08 p.m.

Information

Owner

jzambrano

Repository

Djblets

Branch

master

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

The purpose of this change is to allow enabling/disabling of the '.editable' fields of a review request.

Testing Done

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.

Issues

Description	From	Last Updated
Should be listed alphabetically.	chipx86	Nov. 25, 2012, 4:53 p.m.
Seems redundant with this.options.enable.	chipx86	Dec. 2, 2012, 5:20 p.m.
Only one blank line.	chipx86	Nov. 25, 2012, 4:55 p.m.
You can't have both, and it's a boolean, so just do a standard if/else.	chipx86	Nov. 25, 2012, 4:55 p.m.
No blank line.	chipx86	Nov. 25, 2012, 4:55 p.m.
Add a blank line.	chipx86	Nov. 25, 2012, 4:55 p.m.
I'm not wild about different things controlling this setting. I'm also not sure we should be able to be in …	chipx86	Dec. 2, 2012, 5:20 p.m.
Blank line.	chipx86	Nov. 25, 2012, 4:56 p.m.
It doesn't make sense to go into edit mode when disabled.	chipx86	Dec. 2, 2012, 5:20 p.m.
So, I think Christian was saying before that if we set this to be enabled instead of enable, we can …	mike_conley	Dec. 2, 2012, 4:41 p.m.
Let's not add a trailing comma if we don't need it.	mike_conley	Dec. 1, 2012, 10:12 a.m.
Remove this newline.	mike_conley	Nov. 30, 2012, 9:01 p.m.
So we won't need this anymore, since this.options.enabled will default to true.	mike_conley	Dec. 1, 2012, 10:12 a.m.
This function needs documentation.	mike_conley	Dec. 2, 2012, 4:41 p.m.
Use this.options.enabled instead.	mike_conley	Dec. 1, 2012, 10:13 a.m.
space after if	mike_conley	Nov. 30, 2012, 9:01 p.m.
Set this.options.enabled instead.	mike_conley	Dec. 2, 2012, 4:41 p.m.
This function needs documentation.	mike_conley	Dec. 2, 2012, 4:41 p.m.
use this.options.enabled	mike_conley	Dec. 2, 2012, 4:41 p.m.
Set this.options.enabled	mike_conley	Dec. 2, 2012, 4:41 p.m.
No need for this newline.	mike_conley	Nov. 30, 2012, 8:59 p.m.
Same line	DD ddruska	Dec. 3, 2012, 6:30 a.m.
Blank line.	chipx86	Dec. 3, 2012, 6:09 p.m.
Just a nit - these comments don't need to be broken up so short. 80 chars is our limit, so …	mike_conley	Dec. 2, 2012, 4:44 p.m.
It's true, that's what this does, and that's good info to have, but the summary of this function should be …	chipx86	Dec. 3, 2012, 6:09 p.m.
Here too.	chipx86	Dec. 3, 2012, 6:09 p.m.
Here too.	chipx86	Dec. 3, 2012, 6:10 p.m.
Same as above.	mike_conley	Dec. 2, 2012, 4:44 p.m.
Missing period.	chipx86	Dec. 3, 2012, 6:10 p.m.
Here too.	chipx86	Dec. 3, 2012, 6:10 p.m.
Be careful with your spacing. There should have been a space before "else". Consistency matters.	chipx86	Dec. 4, 2012, 6:46 a.m.
And here.	chipx86	Dec. 4, 2012, 6:46 a.m.

chipx86 Nov. 25, 2012, 4:14 p.m.

It's also important to know step-by-step how you tested it. Every state combination with this should be tested.

djblets/media/js/jquery.gravy.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be listed alphabetically.
```

djblets/media/js/jquery.gravy.js (Diff revision 1)

The issue has been resolved. Show all issues

Seems redundant with this.options.enable.

jzambrano Nov. 30, 2012, 8:41 a.m.

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.

djblets/media/js/jquery.gravy.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Only one blank line.
```

djblets/media/js/jquery.gravy.js (Diff revision 1)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
No blank line.
```
djblets/media/js/jquery.gravy.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Add a blank line.
```

djblets/media/js/jquery.gravy.js (Diff revision 1)

The issue has been resolved. 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.

mike_conley Nov. 26, 2012, 8:13 p.m.

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.

djblets/media/js/jquery.gravy.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line.
```

djblets/media/js/jquery.gravy.js (Diff revision 1)

The issue has been resolved. Show all issues

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

jzambrano Nov. 25, 2012, 5:58 p.m.

I am setting it to true so that startEdit() returns early(so the field is no longer editable)

chipx86 Nov. 26, 2012, 8:08 p.m.

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.

mike_conley Nov. 26, 2012, 8:13 p.m.

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

```
Any updates to this patch, Jesus?
```

Change Summary:

Modified logic of enable and disable functions.

Diff:

Revision 2 (+38 -5)

Show changes

djblets/media/js/jquery.gravy.js

djblets/media/js/jquery.gravy.js (Diff revision 2)

The issue has been resolved. 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.

djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's not add a trailing comma if we don't need it.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Remove this newline.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
So we won't need this anymore, since this.options.enabled will default to true.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This function needs documentation.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Use this.options.enabled instead.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
space after if
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Set this.options.enabled instead.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This function needs documentation.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
use this.options.enabled
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Set this.options.enabled
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for this newline.
```

Change Summary:

Again modified logic of enable/disable functions.

TODO: document enable and disable functions.

Diff:

Revision 3 (+38 -5)

Show changes

djblets/media/js/jquery.gravy.js

Change Summary:

-Inline Documentation for enable and disable functions.
- Modified logic of enable and disable functions.

Diff:

Revision 4 (+35 -4)

Show changes

djblets/media/js/jquery.gravy.js

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?

mike_conley Dec. 3, 2012, 8:43 a.m.

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.

jzambrano Dec. 3, 2012, 7:12 p.m.

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

chipx86 Dec. 3, 2012, 11:36 p.m.

Mike, is this specific with Jesus's patch?

mike_conley Dec. 4, 2012, 6:25 a.m.

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.

djblets/media/js/jquery.gravy.js (Diff revision 4)

The issue has been resolved. 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.

djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Same as above.
```

djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Same line
```

djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line.
```

djblets/media/js/jquery.gravy.js (Diff revision 4)

The issue has been resolved. 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.

djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Here too.
```
1. chipx86 Dec. 3, 2012, 2:30 a.m.
  That is, blank line.
djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Here too.
```
djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing period.
```
djblets/media/js/jquery.gravy.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Here too.
```

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:

	+	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.

Diff:

Revision 5 (+35 -4)

Show changes

djblets/media/js/jquery.gravy.js

Summary:: WIP: change inlineEditor() so it can be enabled/disabled
change inlineEditor() so it can be enabled/disabled

djblets/media/js/jquery.gravy.js (Diff revision 5)

The issue has been resolved. Show all issues

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

djblets/media/js/jquery.gravy.js (Diff revision 5)
The issue has been resolved. Show all issues
```
And here.
```

Change Summary:

Fixed style issues.

Diff:

Revision 6 (+35 -4)

Show changes

djblets/media/js/jquery.gravy.js

Ship it!

```
I'm good with this. Thanks Jesus!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (301e1b1)