Add confirmation when closing diff comment dialog with text present.

Review Request #8410 — Created Sept. 18, 2016 and submitted

stensby
Review Board
release-2.5.x
4373
d3d8005...
reviewboard, students

When pressing ESC or Cancel in the diff viewer comment dialog it would
close and the comment text would be deleted.

Now it will prompt the user if there is text that cancelling will
delete their data.

Tested closing comment box while empty using ESC and cancel,
closing comment box with text using ESC and cancel,
and posting a comment.

Tests done in Chrome 53 and Firefox 48 on Ubuntu 16.04.

Jasmine tests all pass, added 2 unit tests for this change.

Description From Last Updated

Can you rebase this change onto release-2.5.x?

brenniebrennie

Can you wrap your description and testing done at 72 characters?

brenniebrennie

Is it also possible to add a test that checks that if the confirm returns false, that we don't close ...

mike_conleymike_conley

Blank line between these (should insert ones between variable declarations and code, and also surrounding blocks like if, for, etc.).

chipx86chipx86

We use 4 space indentation. This looks to be using two.

chipx86chipx86

This needs to be localized by wrappign the string in gettext(...). Also, we use single quotes instead of double quotes ...

chipx86chipx86

Blank line between these.

chipx86chipx86
ST
chipx86
  1. 
      
  2. Blank line between these (should insert ones between variable declarations and code, and also surrounding blocks like if, for, etc.).

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

    We use 4 space indentation. This looks to be using two.

  4. This needs to be localized by wrappign the string in gettext(...).

    Also, we use single quotes instead of double quotes whenever possible for strings.

  5. Blank line between these.

  6. 
      
ST
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
  2. 
      
brennie
  1. 
      
  2. Can you rebase this change onto release-2.5.x?

  3. Can you wrap your description and testing done at 72 characters?

  4. 
      
ST
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
ST
ST
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. 
      
mike_conley
  1. 
      
  2. Is it also possible to add a test that checks that if the confirm returns false, that we don't close the dialog?

  3. 
      
ST
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. 
      
brennie
  1. This looks good to me!

  2. 
      
david
  1. Ship It!
  2. 
      
brennie
  1. LGTM :)

  2. 
      
ST
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (337e763)
Loading...