1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-22 14:52:41 +01:00

Use an inline dialog element for inline comments in Differential

Summary:
The current approach of using a modal overlay dialog to create/edit inline
comments is pretty silly. Use an inline textarea instead.

This element isn't perfect and we have some mild modalness issues, but I think
it's better than the silly thing we've got going on right now. We can keep
poking it as people break it.

Test Plan:
  - Created comments; submitted and undid them in empty and nonempty states.
Used undo for nonempty states + cancel.
  - Edited comments; saved and canceled them. Used undo for changed state.
  - Replied to comments; yada yada as above.
  - Deleted comments.
  - Did various modal trickery where I clicked "Reply" on something else with a
dialog already up, this very mildly glitches but I think it's not a big issue.

Reviewers: vrana, btrahan, Makinde, nh

Reviewed By: vrana

CC: aran, epriestley

Maniphest Tasks: T431

Differential Revision: https://secure.phabricator.com/D1716
This commit is contained in:
epriestley 2012-02-29 14:28:48 -08:00
parent fe7e991b55
commit 21f0aba701
11 changed files with 375 additions and 87 deletions

View file

@ -1,7 +1,7 @@
<?php
/*
* Copyright 2011 Facebook, Inc.
* Copyright 2012 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.

View file

@ -163,7 +163,7 @@ celerity_register_resource_map(array(
),
'differential-changeset-view-css' =>
array(
'uri' => '/res/2b0c9b6a/rsrc/css/application/differential/changeset-view.css',
'uri' => '/res/38f1bef2/rsrc/css/application/differential/changeset-view.css',
'type' => 'css',
'requires' =>
array(
@ -181,15 +181,16 @@ celerity_register_resource_map(array(
),
'differential-inline-comment-editor' =>
array(
'uri' => '/res/ff5f42a9/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
'uri' => '/res/3fef6fab/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
'type' => 'js',
'requires' =>
array(
0 => 'javelin-dom',
1 => 'javelin-workflow',
2 => 'javelin-util',
3 => 'javelin-stratcom',
4 => 'javelin-install',
1 => 'javelin-util',
2 => 'javelin-stratcom',
3 => 'javelin-install',
4 => 'javelin-request',
5 => 'javelin-workflow',
),
'disk' => '/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
),
@ -503,7 +504,7 @@ celerity_register_resource_map(array(
),
'javelin-behavior-differential-edit-inline-comments' =>
array(
'uri' => '/res/31a8ef7b/rsrc/js/application/differential/behavior-edit-inline-comments.js',
'uri' => '/res/52ce0fe5/rsrc/js/application/differential/behavior-edit-inline-comments.js',
'type' => 'js',
'requires' =>
array(
@ -1931,6 +1932,27 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/46547a92/core.pkg.js',
'type' => 'js',
),
'4876e7f9' =>
array(
'name' => 'differential.pkg.css',
'symbols' =>
array(
0 => 'differential-core-view-css',
1 => 'differential-changeset-view-css',
2 => 'differential-revision-detail-css',
3 => 'differential-revision-history-css',
4 => 'differential-table-of-contents-css',
5 => 'differential-revision-comment-css',
6 => 'differential-revision-add-comment-css',
7 => 'differential-revision-comment-list-css',
8 => 'phabricator-object-selector-css',
9 => 'aphront-headsup-action-list-view-css',
10 => 'phabricator-content-source-view-css',
11 => 'differential-local-commits-view-css',
),
'uri' => '/res/pkg/4876e7f9/differential.pkg.css',
'type' => 'css',
),
'4fbae2af' =>
array(
'name' => 'javelin.pkg.js',
@ -1960,7 +1982,7 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/61f9d480/diffusion.pkg.css',
'type' => 'css',
),
'c8aaade8' =>
'79c97d03' =>
array(
'name' => 'differential.pkg.js',
'symbols' =>
@ -1981,70 +2003,49 @@ celerity_register_resource_map(array(
13 => 'javelin-behavior-phabricator-object-selector',
14 => 'differential-inline-comment-editor',
),
'uri' => '/res/pkg/c8aaade8/differential.pkg.js',
'uri' => '/res/pkg/79c97d03/differential.pkg.js',
'type' => 'js',
),
'e1ccef96' =>
array(
'name' => 'differential.pkg.css',
'symbols' =>
array(
0 => 'differential-core-view-css',
1 => 'differential-changeset-view-css',
2 => 'differential-revision-detail-css',
3 => 'differential-revision-history-css',
4 => 'differential-table-of-contents-css',
5 => 'differential-revision-comment-css',
6 => 'differential-revision-add-comment-css',
7 => 'differential-revision-comment-list-css',
8 => 'phabricator-object-selector-css',
9 => 'aphront-headsup-action-list-view-css',
10 => 'phabricator-content-source-view-css',
11 => 'differential-local-commits-view-css',
),
'uri' => '/res/pkg/e1ccef96/differential.pkg.css',
'type' => 'css',
),
),
'reverse' =>
array(
'aphront-crumbs-view-css' => '29e2c5f1',
'aphront-dialog-view-css' => '29e2c5f1',
'aphront-form-view-css' => '29e2c5f1',
'aphront-headsup-action-list-view-css' => 'e1ccef96',
'aphront-headsup-action-list-view-css' => '4876e7f9',
'aphront-list-filter-view-css' => '29e2c5f1',
'aphront-panel-view-css' => '29e2c5f1',
'aphront-side-nav-view-css' => '29e2c5f1',
'aphront-table-view-css' => '29e2c5f1',
'aphront-tokenizer-control-css' => '29e2c5f1',
'aphront-typeahead-control-css' => '29e2c5f1',
'differential-changeset-view-css' => 'e1ccef96',
'differential-core-view-css' => 'e1ccef96',
'differential-inline-comment-editor' => 'c8aaade8',
'differential-local-commits-view-css' => 'e1ccef96',
'differential-revision-add-comment-css' => 'e1ccef96',
'differential-revision-comment-css' => 'e1ccef96',
'differential-revision-comment-list-css' => 'e1ccef96',
'differential-revision-detail-css' => 'e1ccef96',
'differential-revision-history-css' => 'e1ccef96',
'differential-table-of-contents-css' => 'e1ccef96',
'differential-changeset-view-css' => '4876e7f9',
'differential-core-view-css' => '4876e7f9',
'differential-inline-comment-editor' => '79c97d03',
'differential-local-commits-view-css' => '4876e7f9',
'differential-revision-add-comment-css' => '4876e7f9',
'differential-revision-comment-css' => '4876e7f9',
'differential-revision-comment-list-css' => '4876e7f9',
'differential-revision-detail-css' => '4876e7f9',
'differential-revision-history-css' => '4876e7f9',
'differential-table-of-contents-css' => '4876e7f9',
'diffusion-commit-view-css' => '61f9d480',
'javelin-behavior' => '4fbae2af',
'javelin-behavior-aphront-basic-tokenizer' => '080edee4',
'javelin-behavior-aphront-drag-and-drop' => 'c8aaade8',
'javelin-behavior-aphront-drag-and-drop-textarea' => 'c8aaade8',
'javelin-behavior-aphront-drag-and-drop' => '79c97d03',
'javelin-behavior-aphront-drag-and-drop-textarea' => '79c97d03',
'javelin-behavior-aphront-form-disable-on-submit' => '46547a92',
'javelin-behavior-differential-accept-with-errors' => 'c8aaade8',
'javelin-behavior-differential-add-reviewers-and-ccs' => 'c8aaade8',
'javelin-behavior-differential-comment-jump' => 'c8aaade8',
'javelin-behavior-differential-diff-radios' => 'c8aaade8',
'javelin-behavior-differential-edit-inline-comments' => 'c8aaade8',
'javelin-behavior-differential-feedback-preview' => 'c8aaade8',
'javelin-behavior-differential-keyboard-navigation' => 'c8aaade8',
'javelin-behavior-differential-populate' => 'c8aaade8',
'javelin-behavior-differential-show-more' => 'c8aaade8',
'javelin-behavior-differential-accept-with-errors' => '79c97d03',
'javelin-behavior-differential-add-reviewers-and-ccs' => '79c97d03',
'javelin-behavior-differential-comment-jump' => '79c97d03',
'javelin-behavior-differential-diff-radios' => '79c97d03',
'javelin-behavior-differential-edit-inline-comments' => '79c97d03',
'javelin-behavior-differential-feedback-preview' => '79c97d03',
'javelin-behavior-differential-keyboard-navigation' => '79c97d03',
'javelin-behavior-differential-populate' => '79c97d03',
'javelin-behavior-differential-show-more' => '79c97d03',
'javelin-behavior-phabricator-keyboard-shortcuts' => '46547a92',
'javelin-behavior-phabricator-object-selector' => 'c8aaade8',
'javelin-behavior-phabricator-object-selector' => '79c97d03',
'javelin-behavior-phabricator-watch-anchor' => '46547a92',
'javelin-behavior-refresh-csrf' => '46547a92',
'javelin-behavior-workflow' => '46547a92',
@ -2065,16 +2066,16 @@ celerity_register_resource_map(array(
'javelin-util' => '4fbae2af',
'javelin-vector' => '4fbae2af',
'javelin-workflow' => '46547a92',
'phabricator-content-source-view-css' => 'e1ccef96',
'phabricator-content-source-view-css' => '4876e7f9',
'phabricator-core-buttons-css' => '29e2c5f1',
'phabricator-core-css' => '29e2c5f1',
'phabricator-directory-css' => '29e2c5f1',
'phabricator-drag-and-drop-file-upload' => 'c8aaade8',
'phabricator-drag-and-drop-file-upload' => '79c97d03',
'phabricator-keyboard-shortcut' => '46547a92',
'phabricator-keyboard-shortcut-manager' => '46547a92',
'phabricator-object-selector-css' => 'e1ccef96',
'phabricator-object-selector-css' => '4876e7f9',
'phabricator-remarkup-css' => '29e2c5f1',
'phabricator-shaped-request' => 'c8aaade8',
'phabricator-shaped-request' => '79c97d03',
'phabricator-standard-page-view' => '29e2c5f1',
'syntax-highlighting-css' => '29e2c5f1',
),

View file

@ -228,6 +228,7 @@ phutil_register_library_map(array(
'DifferentialHunk' => 'applications/differential/storage/hunk',
'DifferentialInlineComment' => 'applications/differential/storage/inlinecomment',
'DifferentialInlineCommentEditController' => 'applications/differential/controller/inlinecommentedit',
'DifferentialInlineCommentEditView' => 'applications/differential/view/inlinecommentedit',
'DifferentialInlineCommentPreviewController' => 'applications/differential/controller/inlinecommentpreview',
'DifferentialInlineCommentView' => 'applications/differential/view/inlinecomment',
'DifferentialLinesFieldSpecification' => 'applications/differential/field/specification/lines',
@ -1081,6 +1082,7 @@ phutil_register_library_map(array(
'DifferentialHunk' => 'DifferentialDAO',
'DifferentialInlineComment' => 'DifferentialDAO',
'DifferentialInlineCommentEditController' => 'DifferentialController',
'DifferentialInlineCommentEditView' => 'AphrontView',
'DifferentialInlineCommentPreviewController' => 'DifferentialController',
'DifferentialInlineCommentView' => 'AphrontView',
'DifferentialLinesFieldSpecification' => 'DifferentialFieldSpecification',

View file

@ -40,14 +40,11 @@ class DifferentialInlineCommentEditController extends DifferentialController {
$submit_uri = '/differential/comment/inline/edit/'.$this->revisionID.'/';
$edit_dialog = new AphrontDialogView();
$edit_dialog = new DifferentialInlineCommentEditView();
$edit_dialog->setUser($user);
$edit_dialog->setSubmitURI($submit_uri);
$edit_dialog->addHiddenInput('on_right', $on_right);
$edit_dialog->addSubmitButton();
$edit_dialog->addCancelButton('#');
$edit_dialog->setOnRight($on_right);
switch ($op) {
case 'delete':
@ -58,13 +55,19 @@ class DifferentialInlineCommentEditController extends DifferentialController {
return $this->buildEmptyResponse();
}
$edit_dialog->setTitle('Really delete this comment?');
$edit_dialog->addHiddenInput('id', $inline_id);
$edit_dialog->addHiddenInput('op', 'delete');
$edit_dialog->appendChild(
'<p>Delete this inline comment?</p>');
$dialog = new AphrontDialogView();
$dialog->setUser($user);
$dialog->setSubmitURI($submit_uri);
return id(new AphrontDialogResponse())->setDialog($edit_dialog);
$dialog->setTitle('Really delete this comment?');
$dialog->addHiddenInput('id', $inline_id);
$dialog->addHiddenInput('op', 'delete');
$dialog->appendChild('<p>Delete this inline comment?</p>');
$dialog->addCancelButton('#');
$dialog->addSubmitButton();
return id(new AphrontDialogResponse())->setDialog($dialog);
case 'edit':
$inline = $this->loadInlineCommentForEditing($inline_id);
@ -91,7 +94,8 @@ class DifferentialInlineCommentEditController extends DifferentialController {
$this->renderTextArea(
nonempty($text, $inline->getContent())));
return id(new AphrontDialogResponse())->setDialog($edit_dialog);
return id(new AphrontAjaxResponse())
->setContent($edit_dialog->render());
case 'create':
if (!$request->isFormPost() || !strlen($text)) {
@ -120,7 +124,6 @@ class DifferentialInlineCommentEditController extends DifferentialController {
case 'reply':
default:
if ($op == 'reply') {
$inline = $this->loadInlineComment($inline_id);
// Override defaults.
@ -141,7 +144,8 @@ class DifferentialInlineCommentEditController extends DifferentialController {
$edit_dialog->appendChild($this->renderTextArea($text));
return id(new AphrontDialogResponse())->setDialog($edit_dialog);
return id(new AphrontAjaxResponse())
->setContent($edit_dialog->render());
}
}

View file

@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'applications/differential/storage/changese
phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
phutil_require_module('phabricator', 'applications/differential/storage/revision');
phutil_require_module('phabricator', 'applications/differential/view/inlinecomment');
phutil_require_module('phabricator', 'applications/differential/view/inlinecommentedit');
phutil_require_module('phabricator', 'applications/markup/engine');
phutil_require_module('phabricator', 'applications/phid/handle/data');
phutil_require_module('phabricator', 'infrastructure/javelin/markup');

View file

@ -212,13 +212,17 @@ class DifferentialChangesetListView extends AphrontView {
Javelin::initBehavior('differential-edit-inline-comments', array(
'uri' => '/differential/comment/inline/edit/'.$revision->getID().'/',
'undo_templates' => $undo_templates,
'stage' => 'differential-review-stage',
));
}
return
'<div class="differential-review-stage" id="differential-review-stage">'.
implode("\n", $output).
'</div>';
return phutil_render_tag(
'div',
array(
'class' => 'differential-review-stage',
'id' => 'differential-review-stage',
),
implode("\n", $output));
}
/**

View file

@ -0,0 +1,125 @@
<?php
/*
* Copyright 2012 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
final class DifferentialInlineCommentEditView extends AphrontView {
private $user;
private $inputs = array();
private $uri;
private $title;
private $onRight;
public function addHiddenInput($key, $value) {
$this->inputs[] = array($key, $value);
return $this;
}
public function setUser(PhabricatorUser $user) {
$this->user = $user;
return $this;
}
public function setSubmitURI($uri) {
$this->uri = $uri;
return $this;
}
public function setTitle($title) {
$this->title = $title;
return $this;
}
public function setOnRight($on_right) {
$this->onRight = $on_right;
$this->addHiddenInput('on_right', $on_right);
return $this;
}
public function render() {
if (!$this->uri) {
throw new Exception("Call setSubmitURI() before render()!");
}
if (!$this->user) {
throw new Exception("Call setUser() before render()!");
}
$content = '<th></th><td>'.phabricator_render_form(
$this->user,
array(
'action' => $this->uri,
'method' => 'POST',
'sigil' => 'inline-edit-form',
),
$this->renderInputs().
$this->renderBody()).'</td>';
$other = '<th></th><td></td>';
if ($this->onRight) {
$core = $other.$content;
} else {
$core = $content.$other;
}
return '<table><tr class="inline-comment-splint">'.$core.'</tr></table>';
}
private function renderInputs() {
$out = array();
foreach ($this->inputs as $input) {
list($name, $value) = $input;
$out[] = phutil_render_tag(
'input',
array(
'type' => 'hidden',
'name' => $name,
'value' => $value,
),
null);
}
return implode('', $out);
}
private function renderBody() {
$buttons = array();
$buttons[] = '<button>Submit</button>';
$buttons[] = javelin_render_tag(
'button',
array(
'sigil' => 'inline-edit-cancel',
'class' => 'grey',
),
'Cancel');
$buttons = implode('', $buttons);
return
'<div class="differential-inline-comment-edit">'.
'<div class="differential-inline-comment-edit-title">'.
phutil_escape_html($this->title).
'</div>'.
'<div class="differential-inline-comment-edit-body">'.
$this->renderChildren().
'</div>'.
'<div class="differential-inline-comment-edit-buttons">'.
$buttons.
'<div style="clear: both;"></div>'.
'</div>'.
'</div>';
}
}

View file

@ -0,0 +1,15 @@
<?php
/**
* This file is automatically generated. Lint this module to rebuild it.
* @generated
*/
phutil_require_module('phabricator', 'infrastructure/javelin/markup');
phutil_require_module('phabricator', 'view/base');
phutil_require_module('phutil', 'markup');
phutil_require_source('DifferentialInlineCommentEditView.php');

View file

@ -150,12 +150,13 @@ td.cov-X {
left: 0px;
}
.differential-inline-comment {
.differential-inline-comment,
.differential-inline-comment-edit {
background: #f9f9f1;
border: 1px solid #aaaa88;
font-family: Verdana;
font-size: 11px;
margin: 4px 0px;
margin: 6px 0px;
padding: 8px 10px;
width: 100%;
-moz-box-sizing: border-box;
@ -208,7 +209,7 @@ td.cov-X {
.differential-inline-comment-edit-textarea {
width: 100%;
width: 99%;
height: 12em;
}
@ -280,3 +281,53 @@ td.cov-X {
.differential-inline-undo a {
font-weight: bold;
}
.differential-inline-comment-edit {
padding: 8px 18px 8px 12px;
}
.differential-inline-comment-edit-buttons {
padding: 2px 0px;
}
.differential-inline-comment-edit-buttons button {
float: right;
margin-left: 6px;
}
.differential-inline-comment-edit-title {
font-weight: bold;
color: #333333;
border-bottom: 1px solid #ccccaa;
padding-bottom: 2px;
margin-bottom: 6px;
}
/* When the inline editor is active, disable all the other inline comment links
on the page ("Edit", "Reply", "Delete", etc). The goal here is to prevent
issues where you open up multiple editors and run into problems with
assumptions about modalness. They are disabled explicitly by the JS, but
render them in a disabled state as well.
*/
.inline-editor-active .differential-inline-comment-links a,
.inline-editor-active .differential-inline-comment-links a:hover,
.inline-editor-active .differential-inline-comment-links a:active {
color: #888888;
cursor: normal;
text-decoration: none;
}
/* Create a wide band of color behind the inline edit interface so it is easy
to find by skimming the page while scrolling.
*/
tr.inline-comment-splint {
background: #f9f1d5;
}
tr.differential-inline-hidden {
display: none;
}
tr.differential-inline-loading {
opacity: 0.5;
}

View file

@ -1,10 +1,11 @@
/**
* @provides differential-inline-comment-editor
* @requires javelin-dom
* javelin-workflow
* javelin-util
* javelin-stratcom
* javelin-install
* javelin-request
* javelin-workflow
*/
JX.install('DifferentialInlineCommentEditor', {
@ -70,6 +71,56 @@ JX.install('DifferentialInlineCommentEditor', {
}
JX.DifferentialInlineCommentEditor._activeEditor = this;
},
_setRowState : function(state) {
var is_hidden = (state == 'hidden');
var is_loading = (state == 'loading');
var row = this.getRow();
JX.DOM.alterClass(row, 'differential-inline-hidden', is_hidden);
JX.DOM.alterClass(row, 'differential-inline-loading', is_loading);
},
_didContinueWorkflow : function(response) {
var drawn = this._draw(JX.$N('div', JX.$H(response)));
var op = this.getOperation();
if (op == 'edit') {
this._setRowState('hidden');
}
JX.DOM.find(
drawn[0],
'textarea',
'differential-inline-comment-edit-textarea').focus();
var oncancel = JX.bind(this, function(e) {
e.kill();
this._didCancelWorkflow();
if (op == 'edit') {
this._setRowState('visible');
}
JX.DOM.remove(drawn[0]);
});
JX.DOM.listen(drawn[0], 'click', 'inline-edit-cancel', oncancel);
var onsubmit = JX.bind(this, function(e) {
e.kill();
JX.Workflow.newFromForm(e.getTarget())
.setHandler(JX.bind(this, function(response) {
JX.DOM.remove(drawn[0]);
if (op == 'edit') {
this._setRowState('visible');
}
this._didCompleteWorkflow(response);
}))
.start();
JX.DOM.alterClass(drawn[0], 'differential-inline-loading', true);
});
JX.DOM.listen(drawn[0], 'submit', 'inline-edit-form', onsubmit);
},
_didCompleteWorkflow : function(response) {
var op = this.getOperation();
@ -151,13 +202,33 @@ JX.install('DifferentialInlineCommentEditor', {
this._registerUndoListener();
var data = this._buildRequestData();
var handler = JX.bind(this, this._didCompleteWorkflow);
var close_handler = JX.bind(this, this._didCancelWorkflow);
new JX.Workflow(this._uri, data)
.setHandler(handler)
.setCloseHandler(close_handler)
.start();
var op = this.getOperation();
if (op == 'delete') {
this._setRowState('loading');
var oncomplete = JX.bind(this, this._didCompleteWorkflow);
var onclose = JX.bind(this, function() {
this._setRowState('visible');
this._didCancelWorkflow();
});
new JX.Workflow(this._uri, data)
.setHandler(oncomplete)
.setCloseHandler(onclose)
.start();
} else {
var handler = JX.bind(this, this._didContinueWorkflow);
if (op == 'edit') {
this._setRowState('loading');
}
new JX.Request(this._uri, handler)
.setData(data)
.send();
}
return this;
}

View file

@ -50,6 +50,7 @@ JX.behavior('differential-edit-inline-comments', function(config) {
selecting = false;
editor = false;
hideReticle();
set_link_state(false);
});
function isOnRight(node) {
@ -69,6 +70,10 @@ JX.behavior('differential-edit-inline-comments', function(config) {
}
}
var set_link_state = function(active) {
JX.DOM.alterClass(JX.$(config.stage), 'inline-editor-active', active);
};
JX.Stratcom.listen(
'mousedown',
['differential-changeset', 'tag:th'],
@ -148,6 +153,8 @@ JX.behavior('differential-edit-inline-comments', function(config) {
.setTable(insert.parentNode)
.start();
set_link_state(true);
e.kill();
});
@ -183,9 +190,14 @@ JX.behavior('differential-edit-inline-comments', function(config) {
});
var action_handler = function(op, e) {
e.kill();
if (editor) {
return;
}
var node = e.getNode('differential-inline-comment');
handle_inline_action(node, op);
e.kill();
}
var handle_inline_action = function(node, op) {
@ -208,6 +220,8 @@ JX.behavior('differential-edit-inline-comments', function(config) {
.setRow(row)
.setTable(row.parentNode)
.start();
set_link_state(true);
}
for (var op in {'edit' : 1, 'delete' : 1, 'reply' : 1}) {