jimrrchen got review request #3000!

Summary

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 March 31, 2012, 3:56 p.m.

Information

Owner

jimrrchen

Repository

Djblets

Branch

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

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

Testing Done

Done in Linux environment with Chrome 17 and Firefox 11

Issues

Description	From	Last Updated
We have a dirty() function already. It seems it could just be updated to not assume dirty if in edit …	chipx86	March 24, 2012, 5:45 a.m.
This will break things. There's distinct differences in how codes come in from keydown and keypress, and what will go …	chipx86	March 24, 2012, 5:45 a.m.
All comments must use sentence capitalization and trailing periods. For multi-line comments, use: /* * comment line * ... */	chipx86	March 24, 2012, 5:45 a.m.
So, what does trigger this now? It feels like this will cause API breakage.	chipx86	March 24, 2012, 5:45 a.m.
I know the old code wasn't styled right, but all jquery stuff should be like: $(el) .keydown(function(e) { }) .keyup(function(e) …	chipx86	March 31, 2012, 3:41 p.m.
"resets" Period at the end.	chipx86	March 31, 2012, 3:42 p.m.
Period at the end.	chipx86	March 31, 2012, 3:42 p.m.
No space before ":"	chipx86	March 31, 2012, 3:42 p.m.
I'd prefer "curDirtyState" rather than using "curr".	chipx86	March 31, 2012, 3:43 p.m.

A "dirty" editor is one that has changes in it that need to be saved.
Congrats on review request #3000!

jimrrchen March 23, 2012, 3:08 a.m.

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

Description:: 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

~
Note: I am considering removing the dirty function at line 604, but I am not sure what "$(a).inlineEditor("dirty")" does on line 672. can anyone help?
~
Note: I am considering removing the dirty function at line 604, but I am not sure what "$(a).inlineEditor("dirty")" does on line 672.
+ 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
+ any suggestions?

Fix it!

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

An issue was opened. 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?

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

An issue was opened. 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.

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

An issue was opened. Show all issues

All comments must use sentence capitalization and trailing periods.

For multi-line comments, use:

/*
 * comment line
 * ...
 */

djblets/media/js/jquery.gravy.js (Diff revision 1)
An issue was opened. Show all issues
```
So, what does trigger this now? It feels like this will cause API breakage.
```

Change Summary:

Changed the behaviour of the editor so now whenever the dirty state is changed a "dirtyStateChanged" event is triggered

Description:

		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

~		Note: I am considering removing the dirty function at line 604, but I am not sure what "$(a).inlineEditor("dirty")" does on line 672.
	~	Changed the behaviour of the editor so now whenever the dirty state is changed a "dirtyStateChanged" event is triggered
-		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
-		any suggestions?

Diff:

Revision 2 (+27 -24)

Show changes

djblets/media/js/jquery.gravy.js

Some minor nits. General stuff looks really good.

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

The issue has been resolved. 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) {
    });

djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
"resets"

Period at the end.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Period at the end.
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
No space before ":"
```
djblets/media/js/jquery.gravy.js (Diff revision 2)
The issue has been resolved. Show all issues
```
I'd prefer "curDirtyState" rather than using "curr".
```

Change Summary:

minor styling update according to review comments

Diff:

Revision 3 (+63 -58)

Show changes

djblets/media/js/jquery.gravy.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (8256d90)