revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-19 20:10:55 +01:00
Code Issues Releases Wiki Activity

Make inline comments more discoverable

Browse source 
Summary:
Ref T2009. Right now, when you mouse over a line number, we change the cursor to a "pointer", but that's the only hint we provide about the existence of inline comments.

Occasionally, users have reported confusion around how to leave inline comments.

Try to increase discoverability by showing the line reticle when you hover over the line.

(I could take this or leave it, but it seems OK / not annoying after 15 seconds of playing with it.)

Test Plan: Waved cursor over line numbers, attempted to test all the editor/noncontiguous/across-files cases.

Reviewers: btrahan, chad

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D12010
This commit is contained in:
epriestley 2015-03-07 12:19:45 -08:00
parent 616976a45c
commit 7705f452d2
3 changed files with 93 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
resources/celerity/map.php
View file

@ -10,8 +10,8 @@ return array(
'core.pkg.css' => '6408f2d3',
'core.pkg.js' => '5a1c336d',
'darkconsole.pkg.js' => '8ab24e01',
'differential.pkg.css' => 'df94a9e2',
'differential.pkg.js' => 'f458a7dc',
'differential.pkg.css' => '5f5d3a4c',
'differential.pkg.js' => '58dae818',
'diffusion.pkg.css' => '591664fa',
'diffusion.pkg.js' => 'bfc0737b',
'maniphest.pkg.css' => '68d4dd3d',
@ -55,7 +55,7 @@ return array(
'rsrc/css/application/dashboard/dashboard.css' => '17937d22',
'rsrc/css/application/diff/inline-comment-summary.css' => 'eb5f8e8c',
'rsrc/css/application/differential/add-comment.css' => 'c478bcaa',
'rsrc/css/application/differential/changeset-view.css' => '88100552',
'rsrc/css/application/differential/changeset-view.css' => 'f9011399',
'rsrc/css/application/differential/core.css' => '7ac3cabc',
'rsrc/css/application/differential/results-table.css' => '181aa9d9',
'rsrc/css/application/differential/revision-comment.css' => '48186045',
@ -367,7 +367,7 @@ return array(
'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3',
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '33d4c5e2',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '334267b3',
'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492',
'rsrc/js/application/differential/behavior-populate.js' => '8694b1df',
'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf',
@ -518,7 +518,7 @@ return array(
'conpherence-notification-css' => '04a6e10a',
'conpherence-update-css' => '1099a660',
'conpherence-widget-pane-css' => '3d575438',
'differential-changeset-view-css' => '88100552',
'differential-changeset-view-css' => 'f9011399',
'differential-core-view-css' => '7ac3cabc',
'differential-inline-comment-editor' => '41060c54',
'differential-results-table-css' => '181aa9d9',
@ -569,7 +569,7 @@ return array(
'javelin-behavior-differential-comment-jump' => '4fdb476d',
'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
'javelin-behavior-differential-dropdown-menus' => '2035b9cb',
'javelin-behavior-differential-edit-inline-comments' => '33d4c5e2',
'javelin-behavior-differential-edit-inline-comments' => '334267b3',
'javelin-behavior-differential-feedback-preview' => '6932def3',
'javelin-behavior-differential-keyboard-navigation' => '2c426492',
'javelin-behavior-differential-populate' => '8694b1df',
@ -1027,7 +1027,7 @@ return array(
'331b1611' => array(
'javelin-install',
),
'33d4c5e2' => array(
'334267b3' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',

4
webroot/rsrc/css/application/differential/changeset-view.css
View file

@ -85,6 +85,10 @@
user-select: none;
}
.differential-diff th.selected {
background: {$hovergrey};
}
.differential-changeset-immutable .differential-diff th {
cursor: auto;
}

90
webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
View file

@ -12,6 +12,7 @@ JX.behavior('differential-edit-inline-comments', function(config) {
var selecting = false;
var reticle = JX.$N('div', {className: 'differential-reticle'});
var old_cells = [];
JX.DOM.hide(reticle);
var origin = null;
@ -65,10 +66,44 @@ JX.behavior('differential-edit-inline-comments', function(config) {
dim.setDim(reticle);
JX.DOM.show(reticle);
// Find all the cells in the same row position between the top and bottom
// cell, so we can highlight them.
var seq = 0;
var row = top.parentNode;
for (seq = 0; seq < row.childNodes.length; seq++) {
if (row.childNodes[seq] == top) {
break;
}
}
var cells = [];
while (true) {
cells.push(row.childNodes[seq]);
if (row.childNodes[seq] == bot) {
break;
}
row = row.nextSibling;
}
setSelectedCells(cells);
}
function setSelectedCells(new_cells) {
updateSelectedCellsClass(old_cells, false);
updateSelectedCellsClass(new_cells, true);
old_cells = new_cells;
}
function updateSelectedCellsClass(cells, selected) {
for (var ii = 0; ii < cells.length; ii++) {
JX.DOM.alterClass(cells[ii], 'selected', selected);
}
}
function hideReticle() {
JX.DOM.hide(reticle);
setSelectedCells([]);
}
JX.DifferentialInlineCommentEditor.listen('done', function() {
@ -128,20 +163,59 @@ JX.behavior('differential-edit-inline-comments', function(config) {
});
JX.Stratcom.listen(
'mouseover',
['mouseover', 'mouseout'],
['differential-changeset', 'tag:th'],
function(e) {
if (!selecting ||
editor ||
(getRowNumber(e.getTarget()) === undefined) ||
(isOnRight(e.getTarget()) != isOnRight(origin)) ||
(e.getNode('differential-changeset') !== root)) {
if (editor) {
// Don't update the reticle if we're editing a comment, since this
// would be distracting and we want to keep the lines corresponding
// to the comment highlighted during the edit.
return;
}
target = e.getTarget();
if (getRowNumber(e.getTarget()) === undefined) {
// Don't update the reticle if this "<th />" doesn't correspond to a
// line number. For instance, this may be a dead line number, like the
// empty line numbers on the left hand side of a newly added file.
return;
}
updateReticle();
if (selecting) {
if (isOnRight(e.getTarget()) != isOnRight(origin)) {
// Don't update the reticle if we're selecting a line range and the
// "<th />" under the cursor is on the wrong side of the file. You
// can only leave inline comments on the left or right side of a
// file, not across lines on both sides.
return;
}
if (e.getNode('differential-changeset') !== root) {
// Don't update the reticle if we're selecting a line range and
// the "<th />" under the cursor corresponds to a different file.
// You can only leave inline comments on lines in a single file,
// not across multiple files.
return;
}
}
if (e.getType() == 'mouseout') {
if (selecting) {
// Don't hide the reticle if we're selecting, since we want to
// keep showing the line range that will be used if the mouse is
// released.
return;
}
hideReticle();
} else {
target = e.getTarget();
if (!selecting) {
// If we're just hovering the mouse and not selecting a line range,
// set the origin to the current row so we highlight it.
origin = target;
}
updateReticle();
}
});
JX.Stratcom.listen(