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
2017-09-22 16:17:55 -07:00
parent 1b8631f4dd
commit 355b6b9914
3 changed files with 86 additions and 35 deletions
--- 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)]]"
--- 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';
    },
  });
 })();
--- 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', () => {