Skip to content

Commit 3be3678

Browse files
author
epriestley
committed
Consider inline comments with draft checkmarks as "unsubmitted"
Summary: Ref T12733. When a revision has unsubmitted checkmarks: - Color the banner yellow. - Show them in the "X unsubmitted" count. - Make the "X unsubmitted" button cycle between all drafts (written but unpublished comments) and "draft done" (checked but unsubmitted "Done" checkbox comments). Test Plan: - Checked a "Done" box, saw "1 unsubmitted" and yellow banner. - Clicked "5 unsubmitted" repeatedly, saw it cycle through all unsubmitted comments and checkboxes. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12733 Differential Revision: https://secure.phabricator.com/D18127
1 parent 887bd2d commit 3be3678

File tree

6 files changed

+72
-40
lines changed

6 files changed

+72
-40
lines changed

resources/celerity/map.php

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212
'core.pkg.css' => 'ab24402f',
1313
'core.pkg.js' => '1475bd91',
1414
'darkconsole.pkg.js' => '1f9a31bc',
15-
'differential.pkg.css' => '4e99863c',
16-
'differential.pkg.js' => 'ee50e5ae',
15+
'differential.pkg.css' => '4ec4a37a',
16+
'differential.pkg.js' => '3442216b',
1717
'diffusion.pkg.css' => 'b93d9b8c',
1818
'diffusion.pkg.js' => '6134c5a1',
1919
'favicon.ico' => '30672e08',
@@ -64,7 +64,7 @@
6464
'rsrc/css/application/dashboard/dashboard.css' => 'fe5b1869',
6565
'rsrc/css/application/diff/inline-comment-summary.css' => '51efda3a',
6666
'rsrc/css/application/differential/add-comment.css' => 'c47f8c40',
67-
'rsrc/css/application/differential/changeset-view.css' => 'c72dba88',
67+
'rsrc/css/application/differential/changeset-view.css' => 'b5e6be7f',
6868
'rsrc/css/application/differential/core.css' => '5b7b8ff4',
6969
'rsrc/css/application/differential/phui-inline-comment.css' => 'ffd1a542',
7070
'rsrc/css/application/differential/revision-comment.css' => '14b8565a',
@@ -395,9 +395,9 @@
395395
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
396396
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
397397
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
398-
'rsrc/js/application/diff/DiffChangeset.js' => '94f81a34',
399-
'rsrc/js/application/diff/DiffChangesetList.js' => 'fc6e482d',
400-
'rsrc/js/application/diff/DiffInline.js' => 'a386f83c',
398+
'rsrc/js/application/diff/DiffChangeset.js' => 'cdc5fa19',
399+
'rsrc/js/application/diff/DiffChangesetList.js' => '4ca11264',
400+
'rsrc/js/application/diff/DiffInline.js' => '27b6d01f',
401401
'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
402402
'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07',
403403
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
@@ -562,7 +562,7 @@
562562
'conpherence-thread-manager' => '4d863052',
563563
'conpherence-transaction-css' => '85129c68',
564564
'd3' => 'a11a5ff2',
565-
'differential-changeset-view-css' => 'c72dba88',
565+
'differential-changeset-view-css' => 'b5e6be7f',
566566
'differential-core-view-css' => '5b7b8ff4',
567567
'differential-revision-add-comment-css' => 'c47f8c40',
568568
'differential-revision-comment-css' => '14b8565a',
@@ -774,9 +774,9 @@
774774
'phabricator-darklog' => 'c8e1ffe3',
775775
'phabricator-darkmessage' => 'c48cccdd',
776776
'phabricator-dashboard-css' => 'fe5b1869',
777-
'phabricator-diff-changeset' => '94f81a34',
778-
'phabricator-diff-changeset-list' => 'fc6e482d',
779-
'phabricator-diff-inline' => 'a386f83c',
777+
'phabricator-diff-changeset' => 'cdc5fa19',
778+
'phabricator-diff-changeset-list' => '4ca11264',
779+
'phabricator-diff-inline' => '27b6d01f',
780780
'phabricator-drag-and-drop-file-upload' => '58dea2fa',
781781
'phabricator-draggable-list' => 'bea6e7f4',
782782
'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -1056,6 +1056,9 @@
10561056
'phabricator-drag-and-drop-file-upload',
10571057
'javelin-workboard-board',
10581058
),
1059+
'27b6d01f' => array(
1060+
'javelin-dom',
1061+
),
10591062
'2926fff2' => array(
10601063
'javelin-behavior',
10611064
'javelin-dom',
@@ -1231,6 +1234,10 @@
12311234
'javelin-uri',
12321235
'phabricator-notification',
12331236
),
1237+
'4ca11264' => array(
1238+
'javelin-install',
1239+
'phuix-button-view',
1240+
),
12341241
'4d863052' => array(
12351242
'javelin-dom',
12361243
'javelin-util',
@@ -1612,17 +1619,6 @@
16121619
'javelin-resource',
16131620
'javelin-routable',
16141621
),
1615-
'94f81a34' => array(
1616-
'javelin-dom',
1617-
'javelin-util',
1618-
'javelin-stratcom',
1619-
'javelin-install',
1620-
'javelin-workflow',
1621-
'javelin-router',
1622-
'javelin-behavior-device',
1623-
'javelin-vector',
1624-
'phabricator-diff-inline',
1625-
),
16261622
'960f6a39' => array(
16271623
'javelin-behavior',
16281624
'javelin-dom',
@@ -1667,9 +1663,6 @@
16671663
'javelin-install',
16681664
'javelin-dom',
16691665
),
1670-
'a386f83c' => array(
1671-
'javelin-dom',
1672-
),
16731666
'a3a63478' => array(
16741667
'phui-workcard-view-css',
16751668
),
@@ -1807,6 +1800,9 @@
18071800
'javelin-dom',
18081801
'javelin-util',
18091802
),
1803+
'b5e6be7f' => array(
1804+
'phui-inline-comment-view-css',
1805+
),
18101806
'b6993408' => array(
18111807
'javelin-behavior',
18121808
'javelin-stratcom',
@@ -1900,9 +1896,6 @@
19001896
'javelin-stratcom',
19011897
'javelin-util',
19021898
),
1903-
'c72dba88' => array(
1904-
'phui-inline-comment-view-css',
1905-
),
19061899
'c7ccd872' => array(
19071900
'phui-fontkit-css',
19081901
),
@@ -1963,6 +1956,17 @@
19631956
'cd2b9b77' => array(
19641957
'phui-oi-list-view-css',
19651958
),
1959+
'cdc5fa19' => array(
1960+
'javelin-dom',
1961+
'javelin-util',
1962+
'javelin-stratcom',
1963+
'javelin-install',
1964+
'javelin-workflow',
1965+
'javelin-router',
1966+
'javelin-behavior-device',
1967+
'javelin-vector',
1968+
'phabricator-diff-inline',
1969+
),
19661970
'd0a99ab4' => array(
19671971
'javelin-behavior',
19681972
'javelin-typeahead-ondemand-source',
@@ -2154,10 +2158,6 @@
21542158
'javelin-behavior-device',
21552159
'phabricator-keyboard-shortcut',
21562160
),
2157-
'fc6e482d' => array(
2158-
'javelin-install',
2159-
'phuix-button-view',
2160-
),
21612161
'fc91ab6c' => array(
21622162
'javelin-behavior',
21632163
'javelin-dom',

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,14 @@ public function render() {
107107
break;
108108
}
109109

110+
$is_draft_done = false;
111+
switch ($inline->getFixedState()) {
112+
case PhabricatorInlineCommentInterface::STATE_DRAFT:
113+
case PhabricatorInlineCommentInterface::STATE_UNDRAFT:
114+
$is_draft_done = true;
115+
break;
116+
}
117+
110118
$is_synthetic = false;
111119
if ($inline->getSyntheticAuthor()) {
112120
$is_synthetic = true;
@@ -126,6 +134,7 @@ public function render() {
126134
'isFixed' => $is_fixed,
127135
'isGhost' => $inline->getIsGhost(),
128136
'isSynthetic' => $is_synthetic,
137+
'isDraftDone' => $is_draft_done,
129138
);
130139

131140
$sigil = 'differential-inline-comment';

webroot/rsrc/css/application/differential/changeset-view.css

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,8 @@ tr.differential-inline-loading {
415415
}
416416

417417
.diff-banner-has-unsaved,
418-
.diff-banner-has-unsubmitted {
418+
.diff-banner-has-unsubmitted,
419+
.diff-banner-has-draft-done {
419420
background: {$sh-yellowbackground};
420421
}
421422

webroot/rsrc/js/application/diff/DiffChangeset.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ JX.install('DiffChangeset', {
454454
},
455455
attributes: {
456456
unsaved: inline.isEditing(),
457-
unsubmitted: inline.isDraft(),
457+
anyDraft: inline.isDraft() || inline.isDraftDone(),
458458
undone: (is_saved && !inline.isDone()),
459459
done: (is_saved && inline.isDone())
460460
}

webroot/rsrc/js/application/diff/DiffChangesetList.js

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1332,6 +1332,7 @@ JX.install('DiffChangesetList', {
13321332
var changesets = this._changesets;
13331333
var unsaved = [];
13341334
var unsubmitted = [];
1335+
var draft_done = [];
13351336
var undone = [];
13361337
var done = [];
13371338

@@ -1356,10 +1357,18 @@ JX.install('DiffChangesetList', {
13561357
continue;
13571358
} else if (inline.isDraft()) {
13581359
unsubmitted.push(inline);
1359-
} else if (!inline.isDone()) {
1360-
undone.push(inline);
13611360
} else {
1362-
done.push(inline);
1361+
// NOTE: Unlike other states, an inline may be marked with a
1362+
// draft checkmark and still be a "done" or "undone" comment.
1363+
if (inline.isDraftDone()) {
1364+
draft_done.push(inline);
1365+
}
1366+
1367+
if (!inline.isDone()) {
1368+
undone.push(inline);
1369+
} else {
1370+
done.push(inline);
1371+
}
13631372
}
13641373
}
13651374
}
@@ -1374,6 +1383,11 @@ JX.install('DiffChangesetList', {
13741383
'diff-banner-has-unsubmitted',
13751384
!!unsubmitted.length);
13761385

1386+
JX.DOM.alterClass(
1387+
node,
1388+
'diff-banner-has-draft-done',
1389+
!!draft_done.length);
1390+
13771391
var pht = this.getTranslations();
13781392
var unsaved_button = this._getUnsavedButton();
13791393
var unsubmitted_button = this._getUnsubmittedButton();
@@ -1386,9 +1400,10 @@ JX.install('DiffChangesetList', {
13861400
JX.DOM.hide(unsaved_button.getNode());
13871401
}
13881402

1389-
if (unsubmitted.length) {
1390-
unsubmitted_button.setText(
1391-
unsubmitted.length + ' ' + pht('Unsubmitted'));
1403+
if (unsubmitted.length || draft_done.length) {
1404+
var any_draft_count = unsubmitted.length + draft_done.length;
1405+
1406+
unsubmitted_button.setText(any_draft_count + ' ' + pht('Unsubmitted'));
13921407
JX.DOM.show(unsubmitted_button.getNode());
13931408
} else {
13941409
JX.DOM.hide(unsubmitted_button.getNode());
@@ -1523,7 +1538,7 @@ JX.install('DiffChangesetList', {
15231538
var options = {
15241539
filter: 'comment',
15251540
wrap: true,
1526-
attribute: 'unsubmitted'
1541+
attribute: 'anyDraft'
15271542
};
15281543

15291544
this._onjumpkey(1, options);

webroot/rsrc/js/application/diff/DiffInline.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ JX.install('DiffInline', {
3131

3232
_isCollapsed: false,
3333
_isDraft: null,
34+
_isDraftDone: null,
3435
_isFixed: null,
3536
_isEditing: false,
3637
_isNew: false,
@@ -73,6 +74,7 @@ JX.install('DiffInline', {
7374
this._isFixed = data.isFixed;
7475
this._isGhost = data.isGhost;
7576
this._isSynthetic = data.isSynthetic;
77+
this._isDraftDone = data.isDraftDone;
7678

7779
this._changesetID = data.changesetID;
7880
this._isNew = false;
@@ -103,6 +105,10 @@ JX.install('DiffInline', {
103105
return this._isSynthetic;
104106
},
105107

108+
isDraftDone: function() {
109+
return this._isDraftDone;
110+
},
111+
106112
bindToRange: function(data) {
107113
this._displaySide = data.displaySide;
108114
this._number = parseInt(data.number, 10);
@@ -321,6 +327,7 @@ JX.install('DiffInline', {
321327
JX.DOM.alterClass(comment, 'inline-state-is-draft', response.draftState);
322328

323329
this._isFixed = response.isChecked;
330+
this._isDraftDone = !!response.draftState;
324331

325332
this._didUpdate();
326333
},

0 commit comments

Comments
 (0)