From acf455a9b694031f3830d029888f022e76446e62 Mon Sep 17 00:00:00 2001
From: Becky Siegel <beckysiegel@google.com>
Date: Fri, 9 Dec 2016 11:45:40 -0800
Subject: [PATCH] Make file list more useful on mobile

Previously, the file path was truncated and often the file name was cut
off completely, which had made it hard to tell what files were actually
changed.  With this change, the text appearing on the file list just
show ellipsis and the file name (ex: '.../filename.txt').

Additionally, for both mobile and larger screens, the full filename
appears (line wrapped if needed) when the file list item is expanded.
This way, if enough content is cut off that it's still not useful, there
is a way to see the path in it's entirety.

Bug: Issue 4609
Change-Id: Ic4aaf45bafbc3c5b31add8f7c43b18c9d2b2913b
---
 .../change/gr-file-list/gr-file-list.html     | 23 +++++++++++++++++--
 .../change/gr-file-list/gr-file-list.js       |  9 ++++++++
 .../gr-file-list/gr-file-list_test.html       | 15 ++++++++++++
 .../diff/gr-diff-view/gr-diff-view.js         | 12 +---------
 .../diff/gr-diff-view/gr-diff-view_test.html  |  6 ++---
 polygerrit-ui/app/scripts/util.js             | 20 ++++++++++++++++
 6 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 6425acb7a6..61bc20ba43 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -145,6 +145,12 @@ limitations under the License.
       .patchSetSelect {
         max-width: 8em;
       }
+      .truncatedFileName {
+        display: none;
+      }
+      .expanded .fullFileName {
+        white-space: normal;
+      }
       @media screen and (max-width: 50em) {
         .row.selected {
           background-color: transparent;
@@ -159,6 +165,14 @@ limitations under the License.
         .comments {
           min-width: initial;
         }
+        .expanded .fullFileName,
+        .truncatedFileName {
+          display: block;
+        }
+        .expanded .truncatedFileName,
+        .fullFileName {
+          display: none;
+        }
       }
     </style>
     <header>
@@ -217,11 +231,16 @@ limitations under the License.
         <div class$="[[_computeClass('status', file.__path)]]">
           [[_computeFileStatus(file.status)]]
         </div>
-        <a class="path"
+        <a class$="[[_computePathClass(file.__expanded)]]"
             href$="[[_computeDiffURL(changeNum, patchRange, file.__path)]]">
-          <div title$="[[_computeFileDisplayName(file.__path)]]">
+          <div title$="[[_computeFileDisplayName(file.__path)]]"
+              class="fullFileName">
             [[_computeFileDisplayName(file.__path)]]
           </div>
+          <div title$="[[_computeFileDisplayName(file.__path)]]"
+              class="truncatedFileName">
+            [[_computeTruncatedFileDisplayName(file.__path)]]
+          </div>
           <div class="oldPath" hidden$="[[!file.old_path]]" hidden
               title$="[[file.old_path]]">
             [[file.old_path]]
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index 22a07b0222..b3e8a08cf8 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -512,6 +512,11 @@
       return path === COMMIT_MESSAGE_PATH ? 'Commit message' : path;
     },
 
+    _computeTruncatedFileDisplayName: function(path) {
+      return path === COMMIT_MESSAGE_PATH ?
+          'Commit message' : util.truncatePath(path);
+    },
+
     _formatBytes: function(bytes) {
       if (bytes == 0) return '+/-0 B';
       var bits = 1024;
@@ -545,6 +550,10 @@
       return classes.join(' ');
     },
 
+    _computePathClass: function(expanded) {
+      return expanded ? 'path expanded' : 'path';
+    },
+
     _computeShowHideText: function(expanded) {
       return expanded ? '▼' : '◀';
     },
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index 5da29957b8..5468d5d32d 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -607,5 +607,20 @@ limitations under the License.
         done();
       });
     });
+
+    test('expanded attribute not set on path when not expanded', function() {
+      element._files = [
+        {__path: '/COMMIT_MSG', __expanded: false},
+      ];
+      assert.isNotOk(element.$$('.expanded'));
+    });
+
+    test('expanded attribute set on path when expanded', function() {
+      element._files = [
+        {__path: '/COMMIT_MSG', __expanded: true},
+      ];
+      flushAsynchronousOperations();
+      assert.isOk(element.$$('.expanded'));
+    });
   });
 </script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index f8f646a4e8..1473413fed 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -493,17 +493,7 @@
 
     _computeTruncatedFileDisplayName: function(path) {
       return path === COMMIT_MESSAGE_PATH ?
-          'Commit message' : this._shortenPath(path);
-    },
-
-    _shortenPath: function(path) {
-      var pathPieces = path.split('/');
-
-      if (pathPieces.length < 2) {
-        return path;
-      }
-      // Character is an ellipsis.
-      return '\u2026/' + pathPieces.pop();
+          'Commit message' : util.truncatePath(path);
     },
 
     _computeFileSelected: function(path, currentPath) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index c6ccb1b610..cb8c0956b6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -558,20 +558,20 @@ limitations under the License.
     test('_shortenPath with long path should add ellipsis', function() {
       var path =
           'level1/level2/level3/level4/file.js';
-      var shortenedPath = element._shortenPath(path);
+      var shortenedPath = util.truncatePath(path);
       // The expected path is truncated with an ellipsis.
       var expectedPath = '\u2026/file.js';
       assert.equal(shortenedPath, expectedPath);
 
       var path = 'level2/file.js';
-      var shortenedPath = element._shortenPath(path);
+      var shortenedPath = util.truncatePath(path);
       assert.equal(shortenedPath, expectedPath);
     });
 
     test('_shortenPath with short path should not add ellipsis', function() {
       var path = 'file.js';
       var expectedPath = 'file.js';
-      var shortenedPath = element._shortenPath(path);
+      var shortenedPath = util.truncatePath(path);
       assert.equal(shortenedPath, expectedPath);
     });
 
diff --git a/polygerrit-ui/app/scripts/util.js b/polygerrit-ui/app/scripts/util.js
index 13f3243672..ff518e5f54 100644
--- a/polygerrit-ui/app/scripts/util.js
+++ b/polygerrit-ui/app/scripts/util.js
@@ -55,5 +55,25 @@
     return '';
   };
 
+  /**
+   * Truncates URLs to display filename only
+   * @example
+   * // returns '.../text.html'
+   * util.truncatePath.('dir/text.html');
+   * @example
+   * // returns 'text.html'
+   * util.truncatePath.('text.html');
+   * @returns {String} Returns the truncated value of a URL.
+   */
+  util.truncatePath = function(path) {
+    var pathPieces = path.split('/');
+
+    if (pathPieces.length < 2) {
+      return path;
+    }
+    // Character is an ellipsis.
+    return '\u2026/' + pathPieces[pathPieces.length - 1];
+  };
+
   window.util = util;
 })(window);