Summary

Allow COMMAND+ENTER to save and close the commentDialogView

Review Request #8162 — Created May 13, 2016 and submitted June 27, 2016, 10:02 a.m.

Information

Owner

davidwalsh

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Allowing COMMAND+ENTER to close the commentDialogView widget is more natural to Mac users and is a solution used by many sites for immediate submission. Saves tabbing to the button too.

Testing Done


Command-Enter will save the comment dialog.
Added unit test.

Issues

Description	From	Last Updated
The code here isn't handling Alt-I, despite the comment, but see my next comment anyway.	chipx86	May 17, 2016, 8:53 a.m.
This should be part of the logic in _onTextKeyPress, which is where the control-enter is handled.	chipx86	May 17, 2016, 8:53 a.m.
Indentation should be 4 spaces.	brennie	May 17, 2016, 1:25 p.m.
Can you change this to e.preventDefault(); e.stopPropagation(); ?	brennie	May 17, 2016, 1:25 p.m.
"Enter"	chipx86	May 24, 2016, 7:58 a.m.
This kind of trails off.	chipx86	May 24, 2016, 7:58 a.m.
"minimize." Should include the period at the end (same with other comments).	chipx86	May 24, 2016, 7:58 a.m.
We're in total control of this and everything that calls it, so the !! doesn't add anything useful.	david	June 22, 2016, 9:55 a.m.
Missing period. ``metaKey``	brennie	June 22, 2016, 11:26 a.m.
Same here.	brennie	June 22, 2016, 11:26 a.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

Submitting to gauge interest. COMMAND+ENTER is a popular philosophy for insta-save on Macs. Review Board uses CONTROL+ENTER which is popular on Windows but not Mac.

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 1)

The issue has been resolved. Show all issues

The code here isn't handling Alt-I, despite the comment, but see my next comment anyway.

DA

davidwalsh May 13, 2016, 1:54 p.m.
```
Derp, will update.
```

mike_conley May 16, 2016, 4:49 p.m.

Thanks for the latest revision, David! If you've fixed these issues, can you mark them "Fixed" (or "Drop" them if they no longer apply) - that way, we'll know to do another round of review.

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 1)

The issue has been resolved. Show all issues

This should be part of the logic in _onTextKeyPress, which is where the control-enter is handled.

davidwalsh May 13, 2016, 1:54 p.m.

Event.metakey doens't register in Chrome or Brave with keypress; unfortunately only with keydown

chipx86 May 13, 2016, 1:58 p.m.

Okay. Assuming that's a browser bug, we should make a comment about it there in the code, and, if there's any bug filed about this (I'm seeing discussions online, but haven't checked bugs yet -- kind of heading out right now), we should link to that.

We should also then move the other code for Enter out of the keypress handler and into here, so we're consistent.

I'll have to dig in later to find out why we had all this separated, but I suspect there must have been a reason...

Change Summary:

I've made a few more modifications to allow META key handling for all shortcuts. Let me know what you think of this. Thank you!

Diff:

Revision 2 (+127 -46)

Show changes

	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Indentation should be 4 spaces.
```
reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you change this to e.preventDefault(); e.stopPropagation(); ?
```

Change Summary:

(Updates to satisfy comments)

Diff:

Revision 3 (+107 -25)

Show changes

	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
"Enter"
```
reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This kind of trails off.
```
reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
"minimize."
Should include the period at the end (same with other comments).
```

Change Summary:

Updated nits!

Diff:

Revision 4 (+107 -25)

Show changes

	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

```
Just one small nit left:
```
reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
We're in total control of this and everything that calls it, so the !! doesn't add anything useful.
```

Change Summary:

Removed '!!' as requested!

Diff:

Revision 5 (+107 -25)

Show changes

	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

Ship it!

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Missing period.
``metaKey``
```
reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 5)

Out of curiousily, can Windows + Enter trigger this in Chrome on Windows?

Change Summary:

Fixed nits.

Diff:

Revision 6 (+107 -25)

Show changes

	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

Ship it!

I'm going to make a couple more small changes and push this. Thanks!

Status:: Completed
Change Summary:: Pushed to release-2.5.x (d94cbf6)