From 355b6b9914cc54b163e5ea280fac235488d3b57f Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Fri, 22 Sep 2017 16:17:55 -0700
Subject: [PATCH] Swap 'expand inline' and 'reviewed' controls

In the file list, the 'expand inline' and 'reviewed' controls should be
swapped. Contextually, it makes more sense for the 'expand' control to
appear first, as one needs no information about the file in order to
want to expand it -- in the case of the reviewed checkbox, the user is
expected to have viewed the file before toggling the checkbox state.

In addition, some work was done to migrate the reviewed checkbox to the
new button-like styling designed by Arnab.

Bug: Issue 7275
Change-Id: I5a052da8fdce79523709e9c98b01466cb26eb242
---
 .../change/gr-file-list/gr-file-list.html     | 71 +++++++++++++------
 .../change/gr-file-list/gr-file-list.js       | 22 ++++--
 .../gr-file-list/gr-file-list_test.html       | 28 +++++---
 3 files changed, 86 insertions(+), 35 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 3464fea25e..10f4b27b07 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
@@ -37,6 +37,7 @@ limitations under the License.
         display: block;
       }
       .row {
+        align-items: center;
         border-top: 1px solid #eee;
         display: flex;
         padding: .1em .25em;
@@ -44,6 +45,9 @@ limitations under the License.
       :host(.loading) .row {
         opacity: .5;
       };
+      :host(.editLoaded) .hideOnEdit {
+        display: none;
+      }
       .reviewed,
       .status {
         align-items: center;
@@ -107,14 +111,13 @@ limitations under the License.
         font-family: var(--font-family-bold);
       }
       .show-hide {
-        margin-left: .4em;
+        width: 1em;
       }
       .fileListButton {
         margin: .5em;
       }
       .totalChanges {
         justify-content: flex-end;
-        padding-right: 2.6em;
         text-align: right;
       }
       .warning {
@@ -129,7 +132,6 @@ limitations under the License.
         display: block;
         font-size: .8em;
         min-width: 2em;
-        margin-top: .1em;
       }
       gr-diff {
         box-shadow: 0 1px 3px rgba(0, 0, 0, .3);
@@ -147,12 +149,39 @@ limitations under the License.
       .mobile {
         display: none;
       }
-      #container.editLoaded .hideOnEdit {
-        display: none;
-      }
       .expandInline {
         padding-right: .25em;
       }
+      .reviewed {
+        margin-left: 2em;
+        width: 15em;
+      }
+      .reviewed label {
+        color: #2A66D9;
+        opacity: 0;
+        justify-content: flex-end;
+        width: 100%;
+      }
+      .reviewed label:hover {
+        cursor: pointer;
+        opacity: 100;
+      }
+      .row:hover .reviewed label,
+      .row:focus .reviewed label {
+        opacity: 100;
+      }
+      .reviewed input {
+        display: none;
+      }
+      .reviewedLabel {
+        color: rgba(0, 0, 0, .54);
+        margin-right: 1em;
+        opacity: 0;
+      }
+      .reviewedLabel.isReviewed {
+        display: initial;
+        opacity: 100;
+      }
       @media screen and (max-width: 50em) {
         .desktop {
           display: none;
@@ -185,7 +214,6 @@ limitations under the License.
     </style>
     <div
         id="container"
-        class$="[[_computeContainerClass(editLoaded)]]"
         on-tap="_handleFileListTap">
       <template is="dom-repeat"
           items="[[_shownFiles]]"
@@ -194,13 +222,14 @@ limitations under the License.
           initial-count="[[fileListIncrement]]"
           target-framerate="1">
         <div class="file-row row" data-path$="[[file.__path]]" tabindex="-1">
-          <div class="reviewed hideOnEdit" hidden$="[[!_loggedIn]]" hidden>
-            <input
-                type="checkbox"
-                checked="[[file.isReviewed]]"
-                class="reviewed"
-                aria-label="Reviewed checkbox"
-                title="Mark as reviewed (shortcut: r)">
+          <div class="show-hide" hidden$="[[_userPrefs.expand_inline_diffs]]">
+            <label class="show-hide" data-path$="[[file.__path]]"
+                data-expand=true>
+              <input type="checkbox" class="show-hide"
+                  checked$="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]"
+                  data-path$="[[file.__path]]" data-expand=true>
+              [[_computeShowHideText(file.__path, _expandedFilePaths.*)]]
+            </label>
           </div>
           <div class$="[[_computeClass('status', file.__path)]]"
               tabindex="0"
@@ -261,13 +290,11 @@ limitations under the License.
               [[_formatPercentage(file.size, file.size_delta)]]
             </span>
           </div>
-          <div class="show-hide" hidden$="[[_userPrefs.expand_inline_diffs]]">
-            <label class="show-hide" data-path$="[[file.__path]]"
-                data-expand=true>
-              <input type="checkbox" class="show-hide"
-                  checked$="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]"
-                  data-path$="[[file.__path]]" data-expand=true>
-              [[_computeShowHideText(file.__path, _expandedFilePaths.*)]]
+          <div class="reviewed hideOnEdit" hidden$="[[!_loggedIn]]" hidden>
+            <span class$="reviewedLabel [[_computeReviewedClass(file.isReviewed)]]">Reviewed</span>
+            <label>
+              <input class="reviewed" type="checkbox" checked="[[file.isReviewed]]">
+              <span class="markReviewed" title="Mark as reviewed (shortcut: r)">[[_computeReviewedText(file.isReviewed)]]</span>
             </label>
           </div>
         </div>
@@ -307,6 +334,8 @@ limitations under the License.
           -[[_patchChange.deleted]]
         </span>
       </div>
+      <!-- Empty div here exists to keep spacing in sync with file rows. -->
+      <div class="reviewed hideOnEdit" hidden$="[[!_loggedIn]]" hidden></div>
     </div>
     <div
         class$="row totalChanges [[_computeExpandInlineClass(_userPrefs)]]"
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 100bdee47f..7807f5e160 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
@@ -59,7 +59,10 @@
         notify: true,
         observer: '_updateDiffPreferences',
       },
-      editLoaded: Boolean,
+      editLoaded: {
+        type: Boolean,
+        observer: '_editLoadedChanged',
+      },
       _files: {
         type: Array,
         observer: '_filesChanged',
@@ -424,9 +427,8 @@
         row = row.parentElement;
       }
       const path = row.dataset.path;
-
       // Handle checkbox mark as reviewed.
-      if (e.target.classList.contains('reviewed')) {
+      if (e.target.classList.contains('markReviewed')) {
         return this._reviewFile(path);
       }
 
@@ -720,7 +722,7 @@
     },
 
     _computeShowHideText(path, expandedFilesRecord) {
-      return this._isFileExpanded(path, expandedFilesRecord) ? '▼' : '◀';
+      return this._isFileExpanded(path, expandedFilesRecord) ? '▼' : '▶';
     },
 
     _computeFilesShown(numFilesShown, files) {
@@ -910,8 +912,16 @@
       }, LOADING_DEBOUNCE_INTERVAL);
     },
 
-    _computeContainerClass(editLoaded) {
-      return editLoaded ? 'editLoaded' : '';
+    _editLoadedChanged(editLoaded) {
+      this.classList.toggle('editLoaded', editLoaded);
+    },
+
+    _computeReviewedClass(isReviewed) {
+      return isReviewed ? 'isReviewed' : '';
+    },
+
+    _computeReviewedText(isReviewed) {
+      return isReviewed ? 'MARK UNREVIEWED' : 'MARK REVIEWED';
     },
   });
 })();
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 4b407c456e..e77ad2cbc4 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
@@ -609,21 +609,29 @@ limitations under the License.
       flushAsynchronousOperations();
       const fileRows =
           Polymer.dom(element.root).querySelectorAll('.row:not(.header)');
-      const commitMsg = fileRows[0].querySelector(
-          'input.reviewed[type="checkbox"]');
-      const fileAdded = fileRows[1].querySelector(
-          'input.reviewed[type="checkbox"]');
-      const myFile = fileRows[2].querySelector(
-          'input.reviewed[type="checkbox"]');
+      const checkSelector = 'input.reviewed[type="checkbox"]';
+      const commitMsg = fileRows[0].querySelector(checkSelector);
+      const fileAdded = fileRows[1].querySelector(checkSelector);
+      const myFile = fileRows[2].querySelector(checkSelector);
 
       assert.isTrue(commitMsg.checked);
       assert.isFalse(fileAdded.checked);
       assert.isTrue(myFile.checked);
 
-      MockInteractions.tap(commitMsg);
+      const commitReviewLabel = fileRows[0].querySelector('.reviewedLabel');
+      const markReviewLabel = commitMsg.nextElementSibling;
+      assert.isTrue(commitReviewLabel.classList.contains('isReviewed'));
+      assert.equal(markReviewLabel.textContent, 'MARK UNREVIEWED');
+
+      MockInteractions.tap(markReviewLabel);
       assert.isTrue(saveStub.lastCall.calledWithExactly('/COMMIT_MSG', false));
-      MockInteractions.tap(commitMsg);
+      assert.isFalse(commitReviewLabel.classList.contains('isReviewed'));
+      assert.equal(markReviewLabel.textContent, 'MARK REVIEWED');
+
+      MockInteractions.tap(markReviewLabel);
       assert.isTrue(saveStub.lastCall.calledWithExactly('/COMMIT_MSG', true));
+      assert.isTrue(commitReviewLabel.classList.contains('isReviewed'));
+      assert.equal(markReviewLabel.textContent, 'MARK UNREVIEWED');
     });
 
     test('patch set from revisions', () => {
@@ -1207,12 +1215,15 @@ limitations under the License.
 
       test('reviewed checkbox', () => {
         const alertStub = sandbox.stub();
+        const saveReviewStub = sandbox.stub(element, '_saveReviewedState');
+
         element.addEventListener('show-alert', alertStub);
         element.editLoaded = false;
         // Reviewed checkbox should be shown.
         assert.isTrue(isVisible(element.$$('.reviewed')));
         MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
         assert.isFalse(alertStub.called);
+        assert.isTrue(saveReviewStub.calledOnce);
 
         element.editLoaded = true;
         flushAsynchronousOperations();
@@ -1220,6 +1231,7 @@ limitations under the License.
         assert.isFalse(isVisible(element.$$('.reviewed')));
         MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
         assert.isTrue(alertStub.called);
+        assert.isTrue(saveReviewStub.calledOnce);
       });
 
       test('_getReviewedFiles does not call API', () => {