Add confirmation when closing diff comment dialog with text present.

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

Information

Review Board
release-2.5.x
d3d8005...

Reviewers

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. Show all issues

    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)
     
     
     
     
     
     
     
     
    Show all issues

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

  4. Show all issues

    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. Show all issues

    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. Show all issues

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

  3. Show all issues

    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. Show all issues

    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:
Completed
Change Summary:
Pushed to release-2.5.x (337e763)