From 24ba66f106f2d072aa1ac0fe0c3200cea7bf9e7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 May 2020 06:36:31 -0700 Subject: [PATCH] Persist "Show Changeset" and improve path text selection Summary: Ref T13513. Currently: - If you click the "Show Changeset" button, your state change doesn't actually get saved on the server. - It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event. Instead: persist the former event; make the actual path text not be part of the highlight hitbox. Test Plan: - Clicked "Show Changeset", reloaded, saw changeset visibility persisted. - Selected changeset path text without issues. - Clicked non-text header area to select/deselect changesets. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21236 --- resources/celerity/map.php | 44 +++++++++---------- .../view/DifferentialChangesetDetailView.php | 8 +++- .../differential/changeset-view.css | 5 +++ .../rsrc/js/application/diff/DiffChangeset.js | 17 ++++--- 4 files changed, 45 insertions(+), 29 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 543395b7db..5a84734132 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,8 +12,8 @@ return array( 'core.pkg.css' => '1b80c45d', 'core.pkg.js' => '1e667bcb', 'dark-console.pkg.js' => '187792c2', - 'differential.pkg.css' => '2d70b7b9', - 'differential.pkg.js' => 'faa6e8ab', + 'differential.pkg.css' => 'd71d4531', + 'differential.pkg.js' => 'cf4f3263', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -63,7 +63,7 @@ return array( 'rsrc/css/application/diff/diff-tree-view.css' => 'e2d3e222', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '5fb26c90', + 'rsrc/css/application/differential/changeset-view.css' => 'a5cc67cf', 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -379,7 +379,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', - 'rsrc/js/application/diff/DiffChangeset.js' => 'a49dc31e', + 'rsrc/js/application/diff/DiffChangeset.js' => '700bf848', 'rsrc/js/application/diff/DiffChangesetList.js' => '6992b85c', 'rsrc/js/application/diff/DiffInline.js' => 'db754a7b', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', @@ -560,7 +560,7 @@ return array( 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => '9d068042', 'diff-tree-view-css' => 'e2d3e222', - 'differential-changeset-view-css' => '5fb26c90', + 'differential-changeset-view-css' => 'a5cc67cf', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -776,7 +776,7 @@ return array( 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', - 'phabricator-diff-changeset' => 'a49dc31e', + 'phabricator-diff-changeset' => '700bf848', 'phabricator-diff-changeset-list' => '6992b85c', 'phabricator-diff-inline' => 'db754a7b', 'phabricator-diff-path-view' => '8207abf9', @@ -1489,9 +1489,6 @@ return array( '5faf27b9' => array( 'phuix-form-control-view', ), - '5fb26c90' => array( - 'phui-inline-comment-view-css', - ), '60cd9241' => array( 'javelin-behavior', ), @@ -1555,6 +1552,19 @@ return array( 'javelin-install', 'javelin-util', ), + '700bf848' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + 'phabricator-diff-inline', + 'phabricator-diff-path-view', + 'phuix-button-view', + ), 70245195 => array( 'javelin-behavior', 'javelin-stratcom', @@ -1853,19 +1863,6 @@ return array( 'javelin-stratcom', 'javelin-vector', ), - 'a49dc31e' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - 'phabricator-diff-path-view', - 'phuix-button-view', - ), 'a4aa75c4' => array( 'phui-button-css', 'phui-button-simple-css', @@ -1874,6 +1871,9 @@ return array( 'javelin-install', 'javelin-dom', ), + 'a5cc67cf' => array( + 'phui-inline-comment-view-css', + ), 'a77e2cbd' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php index 5282a793f6..357dc375cf 100644 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -278,7 +278,13 @@ final class DifferentialChangesetDetailView extends AphrontView { ), array( $icon, - $display_filename, + javelin_tag( + 'span', + array( + 'class' => 'differential-changeset-path-name', + 'sigil' => 'changeset-header-path-name', + ), + $display_filename), )), javelin_tag( 'div', diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index c7c27524b4..1442e7411c 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -319,6 +319,11 @@ td.cov-I { cursor: pointer; } +.differential-changeset .differential-file-icon-header + .differential-changeset-path-name { + cursor: auto; +} + .device-phone .differential-changeset .differential-file-icon-header { word-break: break-word; margin-right: 8px; diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index 36022f30b8..a6891dabcb 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -886,6 +886,13 @@ JX.install('DiffChangeset', { }, _onClickHeader: function(e) { + // If the user clicks the actual path name text, don't count this as + // a selection action: we want to let them select the path. + var path_name = e.getNode('changeset-header-path-name'); + if (path_name) { + return; + } + e.prevent(); if (this._isSelected) { @@ -986,18 +993,16 @@ JX.install('DiffChangeset', { _onClickShowButton: function(e) { e.prevent(); - this.setVisible(true); + + // We're always showing the changeset, but want to make sure the state + // change is persisted on the server. + this.toggleVisibility(); }, isVisible: function() { return this._visible; }, - _onundo: function(e) { - e.kill(); - this.toggleVisibility(); - }, - getPathView: function() { if (!this._pathView) { var view = new JX.DiffPathView()