Fix new comment discarding existing unsaved draft comments.

Review Request #9201 — Created Sept. 22, 2017 and submitted

Information

Review Board
release-3.0.x
9d1163e...

Reviewers

When a comment box was open, clicking a new line to add another comment would
discard the current comment without warning or prompting the user to save
changes. The old comment box was silently closed, and a new one was opened.

This change warns the user before the old comment box is closed. Pressing
cancel keeps the old comment box open. Pressing OK closes the old comment
box without saving it, and creates a new one.

Unit tests pass.

Manually tested clicking a new line to add a comment, when a current comment
box was open. Manually tested clicking a new line to add a comment, when a
previously saved comment box was open. Pressing OK continues to create a
new comment, discarding the unsaved comment. Pressing cancel returns to
the current open comment box.

Description From Last Updated

Can you add a unit test for this change? Mentors can help with what's required here.

chipx86chipx86

Col: 40 Expected '!==' and instead saw '!='.

reviewbotreviewbot

A few easy comments on this line: There should be no space between the ! and the confirm Can you …

daviddavid

Please remove this newly-added line (we only want one blank line between class members)

daviddavid

There's now an extra blank line here.

daviddavid

This could be a little more compact by combining these into one conditional: if (this._activeCommentBlock !== null && !confirm(gettext('...'))) {

daviddavid

One last thing: can we wrap this onto two lines to keep it less wide? Split it after the && …

daviddavid

Can I suggest a wording change? How about: "You are currently editing another comment. Would you like to discard it …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

giuliacm
david
  1. 
      
  2. A few easy comments on this line:

    1. There should be no space between the ! and the confirm
    2. Can you use single-quotes instead of double quotes?
    3. Because this is user-visible text, we should mark it as being localizable by wrapping it in gettext(). Thus: confirm(gettext('Do you ... '))
  3. Please remove this newly-added line (we only want one blank line between class members)

  4. 
      
giuliacm
giuliacm
david
  1. 
      
  2. There's now an extra blank line here.

  3. This could be a little more compact by combining these into one conditional:

    if (this._activeCommentBlock !== null &&
        !confirm(gettext('...'))) {
    
  4. 
      
giuliacm
david
  1. 
      
  2. One last thing: can we wrap this onto two lines to keep it less wide? Split it after the &&

    if (this._activeCommentBlock !== null &&
        !confirm('...')) {
    
  3. 
      
giuliacm
chipx86
  1. 
      
  2. Can you add a unit test for this change? Mentors can help with what's required here.

    1. This is one case where I had suggested that creating a unit test was probably not enough benefit for the work. We don't have existing tests for any of this code, so there's no good place to start, and this one requires a huge amount of state set-up to get to the point where writing a test is even possible.

  3. Can I suggest a wording change? How about: "You are currently editing another comment. Would you like to discard it and create a new one?"

    This should also probably check that the other comment's state is "dirty." If they're just viewing another one, I don't think it's worth warning them.

  4. 
      
giuliacm
giuliacm
chipx86
  1. Ship It!
  2. 
      
giuliacm
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (555beef)
Loading...