JSHint
-
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 1) Show all issues
Review Request #9201 — Created Sept. 22, 2017 and submitted
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. |
chipx86 | |
Col: 40 Expected '!==' and instead saw '!='. |
reviewbot | |
A few easy comments on this line: There should be no space between the ! and the confirm Can you … |
david | |
Please remove this newly-added line (we only want one blank line between class members) |
david | |
There's now an extra blank line here. |
david | |
This could be a little more compact by combining these into one conditional: if (this._activeCommentBlock !== null && !confirm(gettext('...'))) { |
david | |
One last thing: can we wrap this onto two lines to keep it less wide? Split it after the && … |
david | |
Can I suggest a wording change? How about: "You are currently editing another comment. Would you like to discard it … |
chipx86 |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+8) |
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 2) |
---|
A few easy comments on this line:
- There should be no space between the
!
and theconfirm
- Can you use single-quotes instead of double quotes?
- Because this is user-visible text, we should mark it as being localizable by wrapping it in
gettext()
. Thus:confirm(gettext('Do you ... '))
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 2) |
---|
Please remove this newly-added line (we only want one blank line between class members)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+7) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+7) |
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 4) |
---|
There's now an extra blank line here.
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 4) |
---|
This could be a little more compact by combining these into one conditional:
if (this._activeCommentBlock !== null && !confirm(gettext('...'))) {
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+4) |
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 5) |
---|
One last thing: can we wrap this onto two lines to keep it less wide? Split it after the
&&
if (this._activeCommentBlock !== null && !confirm('...')) {
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+5) |
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 6) |
---|
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.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+6) |
Testing Done: |
|
---|