From 3c18cb77fb66b19c41f87754f183c15e18c4d32a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 May 2017 13:08:55 -0700 Subject: [PATCH] Move inline "done" checkboxing to DiffInline Summary: Ref T12616. This updates clicking the "Done" checkbox for the new stuff. This one is pretty clean since the "Done" checkbox doesn't do too much weird magic. Test Plan: Clicked the box a few times. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12616 Differential Revision: https://secure.phabricator.com/D17888 --- resources/celerity/map.php | 48 +++++++++---------- .../PhabricatorInlineCommentController.php | 4 ++ .../js/application/diff/DiffChangesetList.js | 9 ++++ .../rsrc/js/application/diff/DiffInline.js | 46 ++++++++++++++++-- .../DifferentialInlineCommentEditor.js | 22 --------- .../behavior-edit-inline-comments.js | 9 +--- 6 files changed, 80 insertions(+), 58 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 40f90cde8c..484d0ca1c5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '2ff7879f', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '58712637', - 'differential.pkg.js' => 'aa750623', + 'differential.pkg.js' => 'e01579c4', 'diffusion.pkg.css' => 'b93d9b8c', 'diffusion.pkg.js' => '84c8f8fd', 'favicon.ico' => '30672e08', @@ -391,14 +391,14 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => '80ac3298', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'ecc9542e', - 'rsrc/js/application/diff/DiffInline.js' => '586c15ff', + 'rsrc/js/application/diff/DiffChangesetList.js' => '12575699', + 'rsrc/js/application/diff/DiffInline.js' => 'bd1b3258', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', - 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '2e3f9738', + 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '9ed8d2b6', 'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d', 'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '974bab6a', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '97f363fc', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => '92904457', 'rsrc/js/application/differential/behavior-populate.js' => '5e41c819', 'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb', @@ -572,7 +572,7 @@ return array( 'd3' => 'a11a5ff2', 'differential-changeset-view-css' => '41af6d25', 'differential-core-view-css' => '5b7b8ff4', - 'differential-inline-comment-editor' => '2e3f9738', + 'differential-inline-comment-editor' => '9ed8d2b6', 'differential-revision-add-comment-css' => 'c47f8c40', 'differential-revision-comment-css' => '14b8565a', 'differential-revision-history-css' => '0e8eb855', @@ -624,7 +624,7 @@ return array( 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', - 'javelin-behavior-differential-edit-inline-comments' => '974bab6a', + 'javelin-behavior-differential-edit-inline-comments' => '97f363fc', 'javelin-behavior-differential-feedback-preview' => 'b064af76', 'javelin-behavior-differential-keyboard-navigation' => '92904457', 'javelin-behavior-differential-populate' => '5e41c819', @@ -786,8 +786,8 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => '80ac3298', - 'phabricator-diff-changeset-list' => 'ecc9542e', - 'phabricator-diff-inline' => '586c15ff', + 'phabricator-diff-changeset-list' => '12575699', + 'phabricator-diff-inline' => 'bd1b3258', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', 'phabricator-fatal-config-template-css' => '8f18fa41', @@ -1000,6 +1000,9 @@ return array( 'javelin-dom', 'javelin-typeahead-normalizer', ), + 12575699 => array( + 'javelin-install', + ), '1499a8cb' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1115,14 +1118,6 @@ return array( 'javelin-install', 'javelin-event', ), - '2e3f9738' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-request', - 'javelin-workflow', - ), '2ee659ce' => array( 'javelin-install', ), @@ -1347,9 +1342,6 @@ return array( 'javelin-vector', 'javelin-dom', ), - '586c15ff' => array( - 'javelin-dom', - ), '58dea2fa' => array( 'javelin-install', 'javelin-util', @@ -1675,7 +1667,7 @@ return array( 'javelin-mask', 'phabricator-drag-and-drop-file-upload', ), - '974bab6a' => array( + '97f363fc' => array( 'javelin-behavior', 'javelin-stratcom', 'javelin-dom', @@ -1711,6 +1703,14 @@ return array( '9d9685d6' => array( 'phui-oi-list-view-css', ), + '9ed8d2b6' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-request', + 'javelin-workflow', + ), '9f36c42d' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1910,6 +1910,9 @@ return array( 'javelin-vector', 'phui-hovercard', ), + 'bd1b3258' => array( + 'javelin-dom', + ), 'bdaf4d04' => array( 'javelin-behavior', 'javelin-dom', @@ -2171,9 +2174,6 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), - 'ecc9542e' => array( - 'javelin-install', - ), 'eded9ee8' => array( 'javelin-behavior', 'javelin-typeahead-ondemand-source', diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 1c92b167b0..1c624c1ad8 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -130,12 +130,14 @@ abstract class PhabricatorInlineCommentController $inline = $this->loadCommentForDone($this->getCommentID()); $is_draft_state = false; + $is_checked = false; switch ($inline->getFixedState()) { case PhabricatorInlineCommentInterface::STATE_DRAFT: $next_state = PhabricatorInlineCommentInterface::STATE_UNDONE; break; case PhabricatorInlineCommentInterface::STATE_UNDRAFT: $next_state = PhabricatorInlineCommentInterface::STATE_DONE; + $is_checked = true; break; case PhabricatorInlineCommentInterface::STATE_DONE: $next_state = PhabricatorInlineCommentInterface::STATE_UNDRAFT; @@ -145,6 +147,7 @@ abstract class PhabricatorInlineCommentController case PhabricatorInlineCommentInterface::STATE_UNDONE: $next_state = PhabricatorInlineCommentInterface::STATE_DRAFT; $is_draft_state = true; + $is_checked = true; break; } @@ -153,6 +156,7 @@ abstract class PhabricatorInlineCommentController return id(new AphrontAjaxResponse()) ->setContent( array( + 'isChecked' => $is_checked, 'draftState' => $is_draft_state, )); case 'delete': diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 701a9f8461..70b360d427 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -29,6 +29,12 @@ JX.install('DiffChangesetList', { 'click', ['differential-inline-comment', 'differential-inline-edit'], onedit); + + var ondone = JX.bind(this, this._ifawake, this._onaction, 'done'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-done'], + ondone); }, properties: { @@ -352,6 +358,9 @@ JX.install('DiffChangesetList', { case 'edit': inline.edit(); break; + case 'done': + inline.toggleDone(); + break; } }, diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index c6bc71a5f4..5566fe0f7c 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -60,7 +60,7 @@ JX.install('DiffInline', { op = 'show'; } - var inline_uri = this._getChangesetList().getInlineURI(); + var inline_uri = this._getInlineURI(); var comment_id = this._id; new JX.Workflow(inline_uri, {op: op, ids: comment_id}) @@ -68,9 +68,46 @@ JX.install('DiffInline', { .start(); }, + toggleDone: function() { + var uri = this._getInlineURI(); + var data = { + op: 'done', + id: this._id + }; + + var ondone = JX.bind(this, this._ondone); + + new JX.Workflow(uri, data) + .setHandler(ondone) + .start(); + }, + + _ondone: function(response) { + var checkbox = JX.DOM.find( + this._row, + 'input', + 'differential-inline-done'); + + checkbox.checked = (response.isChecked ? 'checked' : null); + + var comment = JX.DOM.findAbove( + checkbox, + 'div', + 'differential-inline-comment'); + + JX.DOM.alterClass(comment, 'inline-is-done', response.isChecked); + + // NOTE: This is marking the inline as having an unsubmitted checkmark, + // as opposed to a submitted checkmark. This is different from the + // top-level "draft" state of unsubmitted comments. + JX.DOM.alterClass(comment, 'inline-state-is-draft', response.draftState); + + this._didUpdate(); + }, + edit: function() { var handler = JX.bind(this, this._oneditresponse); - var uri = this.getChangeset().getChangesetList().getInlineURI(); + var uri = this._getInlineURI(); var data = this._newRequestData(); // TODO: Set state to "loading". @@ -221,9 +258,10 @@ JX.install('DiffInline', { } }, - _getChangesetList: function() { + _getInlineURI: function() { var changeset = this.getChangeset(); - return changeset.getChangesetList(); + var list = changeset.getChangesetList(); + return list.getInlineURI(); } } diff --git a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js index 7e6015d969..7068e94c14 100644 --- a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js +++ b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js @@ -312,28 +312,6 @@ JX.install('DifferentialInlineCommentEditor', { .start(); }, - toggleCheckbox: function(id, checkbox) { - var data = { - op: 'done', - id: id - }; - - new JX.Workflow(this._uri, data) - .setHandler(JX.bind(this, function(r) { - checkbox.checked = !checkbox.checked; - - var comment = JX.DOM.findAbove( - checkbox, - 'div', - 'differential-inline-comment'); - JX.DOM.alterClass(comment, 'inline-is-done', !!checkbox.checked); - JX.DOM.alterClass(comment, 'inline-state-is-draft', r.draftState); - - this._didUpdate(); - })) - .start(); - }, - _didUpdate: function() { // After making changes to inline comments, refresh the transaction // preview at the bottom of the page. diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 904d8f655e..20eec1a3d5 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -371,13 +371,6 @@ JX.behavior('differential-edit-inline-comments', function(config) { } } - if (op == 'done') { - var checkbox = JX.DOM.find(node, 'input', 'differential-inline-done'); - new JX.DifferentialInlineCommentEditor(config.uri) - .toggleCheckbox(data.id, checkbox); - return; - } - var original = data.original; var reply_phid = null; if (op == 'reply') { @@ -417,7 +410,7 @@ JX.behavior('differential-edit-inline-comments', function(config) { set_link_state(true); }; - for (var op in {'edit': 1, 'delete': 1, 'reply': 1, 'done': 1}) { + for (var op in {'edit': 1, 'delete': 1, 'reply': 1}) { JX.Stratcom.listen( 'click', ['differential-inline-comment', 'differential-inline-' + op],