mirror of
https://we.phorge.it/source/phorge.git
synced 2025-01-29 16:08:22 +01:00
Move inline comment creation to new DiffInline code
Summary: Ref T12616. This makes creating inlines use the new code. Creation and editing is now slightly more consistent in how it uses nodes. This will simplify the next change (replies), which I ran into some trouble with in an earlier iteration. Note that this (and other changes in the series) allow you to create and edit multiple inlines simultaneously. This is mostly a feature, although I expect we'll need to lock it down a little bit. I have some UI ideas to help avoid errors. Test Plan: Created inlines on a single line; on a range of lines; on the same line; multiple inlines at the same time. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12616 Differential Revision: https://secure.phabricator.com/D17893
This commit is contained in:
epriestley
parent
d97f80bc90
commit
58dded555b
4 changed files with 171 additions and 73 deletions
|
|
@ -13,7 +13,7 @@ return array(
|
|||
'core.pkg.js' => '2ff7879f',
|
||||
'darkconsole.pkg.js' => '1f9a31bc',
|
||||
'differential.pkg.css' => '58712637',
|
||||
'differential.pkg.js' => '271a1e1e',
|
||||
'differential.pkg.js' => '3a7c5866',
|
||||
'diffusion.pkg.css' => 'b93d9b8c',
|
||||
'diffusion.pkg.js' => '84c8f8fd',
|
||||
'favicon.ico' => '30672e08',
|
||||
|
|
@ -390,15 +390,15 @@ return array(
|
|||
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
|
||||
'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' => '57b29223',
|
||||
'rsrc/js/application/diff/DiffChangeset.js' => '454cfe59',
|
||||
'rsrc/js/application/diff/DiffChangesetList.js' => '50bc5b50',
|
||||
'rsrc/js/application/diff/DiffInline.js' => '64dfc791',
|
||||
'rsrc/js/application/diff/DiffInline.js' => '38a957be',
|
||||
'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
|
||||
'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' => '9d9dbc38',
|
||||
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '995c805a',
|
||||
'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',
|
||||
|
|
@ -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' => '9d9dbc38',
|
||||
'javelin-behavior-differential-edit-inline-comments' => '995c805a',
|
||||
'javelin-behavior-differential-feedback-preview' => 'b064af76',
|
||||
'javelin-behavior-differential-keyboard-navigation' => '92904457',
|
||||
'javelin-behavior-differential-populate' => '5e41c819',
|
||||
|
|
@ -785,9 +785,9 @@ return array(
|
|||
'phabricator-darklog' => 'c8e1ffe3',
|
||||
'phabricator-darkmessage' => 'c48cccdd',
|
||||
'phabricator-dashboard-css' => 'fe5b1869',
|
||||
'phabricator-diff-changeset' => '57b29223',
|
||||
'phabricator-diff-changeset' => '454cfe59',
|
||||
'phabricator-diff-changeset-list' => '50bc5b50',
|
||||
'phabricator-diff-inline' => '64dfc791',
|
||||
'phabricator-diff-inline' => '38a957be',
|
||||
'phabricator-drag-and-drop-file-upload' => '58dea2fa',
|
||||
'phabricator-draggable-list' => 'bea6e7f4',
|
||||
'phabricator-fatal-config-template-css' => '8f18fa41',
|
||||
|
|
@ -1132,6 +1132,9 @@ return array(
|
|||
'javelin-dom',
|
||||
'javelin-workflow',
|
||||
),
|
||||
'38a957be' => array(
|
||||
'javelin-dom',
|
||||
),
|
||||
'3ab51e2c' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-behavior-device',
|
||||
|
|
@ -1211,6 +1214,17 @@ return array(
|
|||
'javelin-behavior',
|
||||
'javelin-dom',
|
||||
),
|
||||
'454cfe59' => array(
|
||||
'javelin-dom',
|
||||
'javelin-util',
|
||||
'javelin-stratcom',
|
||||
'javelin-install',
|
||||
'javelin-workflow',
|
||||
'javelin-router',
|
||||
'javelin-behavior-device',
|
||||
'javelin-vector',
|
||||
'phabricator-diff-inline',
|
||||
),
|
||||
'469c0d9e' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-dom',
|
||||
|
|
@ -1342,17 +1356,6 @@ return array(
|
|||
'javelin-vector',
|
||||
'javelin-dom',
|
||||
),
|
||||
'57b29223' => array(
|
||||
'javelin-dom',
|
||||
'javelin-util',
|
||||
'javelin-stratcom',
|
||||
'javelin-install',
|
||||
'javelin-workflow',
|
||||
'javelin-router',
|
||||
'javelin-behavior-device',
|
||||
'javelin-vector',
|
||||
'phabricator-diff-inline',
|
||||
),
|
||||
'58dea2fa' => array(
|
||||
'javelin-install',
|
||||
'javelin-util',
|
||||
|
|
@ -1422,9 +1425,6 @@ return array(
|
|||
'javelin-workflow',
|
||||
'javelin-dom',
|
||||
),
|
||||
'64dfc791' => array(
|
||||
'javelin-dom',
|
||||
),
|
||||
'680ea2c8' => array(
|
||||
'javelin-install',
|
||||
'javelin-dom',
|
||||
|
|
@ -1675,6 +1675,14 @@ return array(
|
|||
'javelin-dom',
|
||||
'javelin-reactor-dom',
|
||||
),
|
||||
'995c805a' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-stratcom',
|
||||
'javelin-dom',
|
||||
'javelin-util',
|
||||
'javelin-vector',
|
||||
'differential-inline-comment-editor',
|
||||
),
|
||||
'9a6dd75c' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-stratcom',
|
||||
|
|
@ -1698,14 +1706,6 @@ return array(
|
|||
'9d9685d6' => array(
|
||||
'phui-oi-list-view-css',
|
||||
),
|
||||
'9d9dbc38' => array(
|
||||
'javelin-behavior',
|
||||
'javelin-stratcom',
|
||||
'javelin-dom',
|
||||
'javelin-util',
|
||||
'javelin-vector',
|
||||
'differential-inline-comment-editor',
|
||||
),
|
||||
'9ed8d2b6' => array(
|
||||
'javelin-dom',
|
||||
'javelin-util',
|
||||
|
|
|
|||
|
|
@ -411,16 +411,28 @@ JX.install('DiffChangeset', {
|
|||
var data = JX.Stratcom.getData(node);
|
||||
|
||||
if (!data.inline) {
|
||||
var inline = new JX.DiffInline(node)
|
||||
.setChangeset(this);
|
||||
var inline = new JX.DiffInline()
|
||||
.setChangeset(this)
|
||||
.bindToRow(node);
|
||||
|
||||
this._inlines.push(inline);
|
||||
data.inline = inline;
|
||||
}
|
||||
|
||||
return data.inline;
|
||||
},
|
||||
|
||||
newInlineForRange: function(data) {
|
||||
var inline = new JX.DiffInline()
|
||||
.setChangeset(this)
|
||||
.bindToRange(data);
|
||||
|
||||
this._inlines.push(inline);
|
||||
|
||||
inline.create();
|
||||
|
||||
return inline;
|
||||
},
|
||||
|
||||
getInlineByID: function(id) {
|
||||
// TODO: Currently, this will only find inlines which the user has
|
||||
// already interacted with! Inlines are built lazily as events arrive.
|
||||
|
|
|
|||
|
|
@ -6,33 +6,7 @@
|
|||
|
||||
JX.install('DiffInline', {
|
||||
|
||||
construct : function(row) {
|
||||
this._row = row;
|
||||
|
||||
var row_data = JX.Stratcom.getData(row);
|
||||
this._hidden = row_data.hidden || false;
|
||||
|
||||
// TODO: Get smarter about this once we do more editing, this is pretty
|
||||
// hacky.
|
||||
var comment = JX.DOM.find(row, 'div', 'differential-inline-comment');
|
||||
var data = JX.Stratcom.getData(comment);
|
||||
|
||||
this._id = data.id;
|
||||
|
||||
// TODO: This is very, very, very, very, very, very, very hacky.
|
||||
var td = comment.parentNode;
|
||||
var th = td.previousSibling;
|
||||
if (th.parentNode.firstChild != th) {
|
||||
this._displaySide = 'right';
|
||||
} else {
|
||||
this._displaySide = 'left';
|
||||
}
|
||||
|
||||
this._number = data.number;
|
||||
this._length = data.length;
|
||||
this._isNewFile =
|
||||
(this.getDisplaySide() == 'right') ||
|
||||
(data.left != data.right);
|
||||
construct : function() {
|
||||
},
|
||||
|
||||
properties: {
|
||||
|
|
@ -41,6 +15,8 @@ JX.install('DiffInline', {
|
|||
|
||||
members: {
|
||||
_id: null,
|
||||
_phid: null,
|
||||
_changesetID: null,
|
||||
_row: null,
|
||||
_hidden: false,
|
||||
_number: null,
|
||||
|
|
@ -53,6 +29,80 @@ JX.install('DiffInline', {
|
|||
_isInvisible: false,
|
||||
_isLoading: false,
|
||||
|
||||
bindToRow: function(row) {
|
||||
this._row = row;
|
||||
|
||||
var row_data = JX.Stratcom.getData(row);
|
||||
row_data.inline = this;
|
||||
|
||||
this._hidden = row_data.hidden || false;
|
||||
|
||||
// TODO: Get smarter about this once we do more editing, this is pretty
|
||||
// hacky.
|
||||
var comment = JX.DOM.find(row, 'div', 'differential-inline-comment');
|
||||
var data = JX.Stratcom.getData(comment);
|
||||
|
||||
this._id = data.id;
|
||||
this._phid = data.phid;
|
||||
|
||||
// TODO: This is very, very, very, very, very, very, very hacky.
|
||||
var td = comment.parentNode;
|
||||
var th = td.previousSibling;
|
||||
if (th.parentNode.firstChild != th) {
|
||||
this._displaySide = 'right';
|
||||
} else {
|
||||
this._displaySide = 'left';
|
||||
}
|
||||
|
||||
this._number = data.number;
|
||||
this._length = data.length;
|
||||
this._isNewFile =
|
||||
(this.getDisplaySide() == 'right') ||
|
||||
(data.left != data.right);
|
||||
|
||||
this.setInvisible(false);
|
||||
|
||||
return this;
|
||||
},
|
||||
|
||||
bindToRange: function(data) {
|
||||
this._id = null;
|
||||
this._phid = null;
|
||||
|
||||
this._hidden = false;
|
||||
|
||||
this._displaySide = data.displaySide;
|
||||
this._number = data.number;
|
||||
this._length = data.length;
|
||||
this._isNewFile = data.isNewFile;
|
||||
this._changesetID = data.changesetID;
|
||||
|
||||
var row = this._newRow();
|
||||
JX.Stratcom.getData(row).inline = this;
|
||||
this._row = row;
|
||||
|
||||
this.setInvisible(true);
|
||||
|
||||
// Insert the comment after any other comments which already appear on
|
||||
// the same row.
|
||||
var parent_row = JX.DOM.findAbove(data.target, 'tr');
|
||||
var target_row = parent_row.nextSibling;
|
||||
while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
|
||||
target_row = target_row.nextSibling;
|
||||
}
|
||||
parent_row.parentNode.insertBefore(row, target_row);
|
||||
|
||||
return this;
|
||||
},
|
||||
|
||||
_newRow: function() {
|
||||
var attributes = {
|
||||
sigil: 'inline-row'
|
||||
};
|
||||
|
||||
return JX.$N('tr', attributes);
|
||||
},
|
||||
|
||||
setHidden: function(hidden) {
|
||||
this._hidden = hidden;
|
||||
|
||||
|
|
@ -110,6 +160,18 @@ JX.install('DiffInline', {
|
|||
this._didUpdate();
|
||||
},
|
||||
|
||||
create: function() {
|
||||
var uri = this._getInlineURI();
|
||||
var handler = JX.bind(this, this._oncreateresponse);
|
||||
var data = this._newRequestData('new');
|
||||
|
||||
this.setLoading(true);
|
||||
|
||||
new JX.Request(uri, handler)
|
||||
.setData(data)
|
||||
.send();
|
||||
},
|
||||
|
||||
edit: function() {
|
||||
var uri = this._getInlineURI();
|
||||
var handler = JX.bind(this, this._oneditresponse);
|
||||
|
|
@ -172,6 +234,10 @@ JX.install('DiffInline', {
|
|||
return this._id;
|
||||
},
|
||||
|
||||
getChangesetID: function() {
|
||||
return this._changesetID;
|
||||
},
|
||||
|
||||
setDeleted: function(deleted) {
|
||||
this._isDeleted = deleted;
|
||||
this._redraw();
|
||||
|
|
@ -199,6 +265,7 @@ JX.install('DiffInline', {
|
|||
number: this.getLineNumber(),
|
||||
length: this.getLineLength(),
|
||||
is_new: this.isNewFile(),
|
||||
changesetID: this.getChangesetID(),
|
||||
replyToCommentPHID: ''
|
||||
};
|
||||
},
|
||||
|
|
@ -212,6 +279,12 @@ JX.install('DiffInline', {
|
|||
this.setInvisible(true);
|
||||
},
|
||||
|
||||
_oncreateresponse: function(response) {
|
||||
var rows = JX.$H(response).getNode();
|
||||
|
||||
this._drawEditRows(rows);
|
||||
},
|
||||
|
||||
_ondeleteresponse: function() {
|
||||
this._drawUndoRows();
|
||||
|
||||
|
|
@ -251,7 +324,7 @@ JX.install('DiffInline', {
|
|||
// into the document.
|
||||
next_row = row.nextSibling;
|
||||
|
||||
cursor.parentNode.insertBefore(row, cursor.nextSibling);
|
||||
cursor.parentNode.insertBefore(row, cursor);
|
||||
cursor = row;
|
||||
|
||||
var row_meta = {
|
||||
|
|
@ -287,6 +360,17 @@ JX.install('DiffInline', {
|
|||
JX.bind(this, this._onundo, row_meta)));
|
||||
}
|
||||
|
||||
// If the row has a textarea, focus it. This allows the user to start
|
||||
// typing a comment immediately after a "new", "edit", or "reply"
|
||||
// action.
|
||||
var textareas = JX.DOM.scry(
|
||||
row,
|
||||
'textarea',
|
||||
'differential-inline-comment-edit-textarea');
|
||||
if (textareas.length) {
|
||||
textareas[0].focus();
|
||||
}
|
||||
|
||||
row = next_row;
|
||||
}
|
||||
|
||||
|
|
@ -340,6 +424,7 @@ JX.install('DiffInline', {
|
|||
_onsubmitresponse: function(row, response) {
|
||||
this._removeRow(row);
|
||||
|
||||
this.setLoading(false);
|
||||
this.setInvisible(false);
|
||||
|
||||
this._onupdate(response);
|
||||
|
|
@ -358,7 +443,7 @@ JX.install('DiffInline', {
|
|||
JX.DOM.remove(this._row);
|
||||
}
|
||||
|
||||
this._row = new_row;
|
||||
this.bindToRow(new_row);
|
||||
|
||||
this._didUpdate();
|
||||
},
|
||||
|
|
|
|||
|
|
@ -276,18 +276,19 @@ JX.behavior('differential-edit-inline-comments', function(config) {
|
|||
|
||||
var view = JX.DiffChangeset.getForNode(root);
|
||||
|
||||
editor = new JX.DifferentialInlineCommentEditor(config.uri)
|
||||
.setTemplates(view.getUndoTemplates())
|
||||
.setOperation('new')
|
||||
.setChangesetID(changeset)
|
||||
.setLineNumber(o)
|
||||
.setLength(len)
|
||||
.setIsNew(isNewFile(target) ? 1 : 0)
|
||||
.setOnRight(isOnRight(target) ? 1 : 0)
|
||||
.setRow(insert.nextSibling)
|
||||
.setTable(insert.parentNode)
|
||||
.setRenderer(view.getRenderer())
|
||||
.start();
|
||||
view.newInlineForRange({
|
||||
origin: origin,
|
||||
target: target,
|
||||
number: o,
|
||||
length: len,
|
||||
changesetID: changeset,
|
||||
isNewFile: isNewFile(target),
|
||||
displaySide: isOnRight(target) ? 'right' : 'left'
|
||||
});
|
||||
|
||||
selecting = false;
|
||||
origin = null;
|
||||
target = null;
|
||||
|
||||
set_link_state(true);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue