• 
      

    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)