Allow COMMAND+ENTER to save and close the commentDialogView

Review Request #8162 — Created May 13, 2016 and submitted

davidwalsh
Review Board
reviewboard

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.

  • Command-Enter will save the comment dialog.
  • Added unit test.
Description From Last Updated

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

chipx86chipx86

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

chipx86chipx86

Indentation should be 4 spaces.

brenniebrennie

Can you change this to e.preventDefault(); e.stopPropagation(); ?

brenniebrennie

"Enter"

chipx86chipx86

This kind of trails off.

chipx86chipx86

"minimize." Should include the period at the end (same with other comments).

chipx86chipx86

We're in total control of this and everything that calls it, so the !! doesn't add anything useful.

daviddavid

Missing period. ``metaKey``

brenniebrennie

Same here.

brenniebrennie
reviewbot
  1. 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
    
    
  2. 
      
DA
  1. 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.

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

    1. Derp, will update.

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

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

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

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

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

  4. 
      
DA
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 2)
     
     
     
     
     

    Indentation should be 4 spaces.

  3. Can you change this to e.preventDefault(); e.stopPropagation(); ?

  4. 
      
DA
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. This kind of trails off.

  3. "minimize."

    Should include the period at the end (same with other comments).

  4. 
      
DA
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Just one small nit left:

  2. We're in total control of this and everything that calls it, so the !! doesn't add anything useful.

  3. 
      
DA
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Missing period.

    ``metaKey``
    
  3. Out of curiousily, can Windows + Enter trigger this in Chrome on Windows?

  4. 
      
DA
reviewbot
  1. 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
    
    
  2. 
      
david
  1. I'm going to make a couple more small changes and push this. Thanks!

  2. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (d94cbf6)
Loading...