Merge "Add the thread list view in gr-change-view"

2018-03-13 17:00:14 +00:00
parent 9ad3f7cc54 498120df43
commit 63489d858c
5 changed files with 125 additions and 9 deletions
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -18,6 +18,7 @@ limitations under the License.
 <link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
+<link rel="import" href="../../../bower_components/paper-tabs/paper-tabs.html">
 <link rel="import" href="../../../styles/shared-styles.html">
 <link rel="import" href="../../core/gr-navigation/gr-navigation.html">
 <link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
@@ -44,6 +45,7 @@ limitations under the License.
 <link rel="import" href="../gr-messages-list/gr-messages-list.html">
 <link rel="import" href="../gr-related-changes-list/gr-related-changes-list.html">
 <link rel="import" href="../gr-reply-dialog/gr-reply-dialog.html">
+<link rel="import" href="../gr-thread-list/gr-thread-list.html">

 <dom-module id="gr-change-view">
  <template>
@@ -239,6 +241,24 @@ limitations under the License.
        display: inline-block;
        margin-right: -5px;
      }
+      paper-tabs {
+        background-color: #fafafa;
+        border-top: 1px solid #ddd;
+        height: 3rem;
+        --paper-tabs-selection-bar-color: var(--color-link);
+      }
+      paper-tab {
+        max-width: 15rem;
+        --paper-tab-ink: var(--color-link);
+      }
+      #threadList,
+      #messageList {
+        display: none;
+      }
+      #threadList.visible,
+      #messageList.visible {
+        display: block;
+      }
      @media screen and (min-width: 80em) {
        .commitMessage {
          max-width: var(--commit-message-max-width, 100ch);
@@ -526,8 +546,14 @@ limitations under the License.
        <gr-endpoint-param name="revision" value="[[_selectedRevision]]">
        </gr-endpoint-param>
      </gr-endpoint-decorator>
+      <paper-tabs
+          id="commentTabs"
+          on-selected-changed="_handleTabChange">
+        <paper-tab class="changeLog">Change Log</paper-tab>
+        <paper-tab class="commentThreads">Comment Threads</paper-tab>
+      </paper-tabs>
      <gr-messages-list id="messageList"
-          class="hideOnMobileOverlay"
+          class$="hideOnMobileOverlay [[_computeShowMessages(_showMessagesView)]]"
          change-num="[[_changeNum]]"
          labels="[[_change.labels]]"
          messages="[[_change.messages]]"
@@ -536,6 +562,13 @@ limitations under the License.
          project-name="[[_change.project]]"
          show-reply-buttons="[[_loggedIn]]"
          on-reply="_handleMessageReply"></gr-messages-list>
+      <gr-thread-list
+          id="threadList"
+          threads="[[_commentThreads]]"
+          change="[[_change]]"
+          change-num="[[_changeNum]]"
+          class$="[[_computeShowThreads(_showMessagesView)]]"
+          on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list>
    </div>
    <gr-overlay id="downloadOverlay" with-backdrop>
      <gr-download-dialog
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -228,6 +228,10 @@
        type: Boolean,
        value: undefined,
      },
+      _showMessagesView: {
+        type: Boolean,
+        value: true,
+      },
    },

    behaviors: [
@@ -246,7 +250,6 @@
      'fullscreen-overlay-opened': '_handleHideBackgroundContent',
      'fullscreen-overlay-closed': '_handleShowBackgroundContent',
      'diff-comments-modified': '_handleReloadCommentThreads',
-      'thread-list-modified': '_handleReloadDiffComments',
    },
    observers: [
      '_labelsChanged(_change.labels.*)',
@@ -331,6 +334,18 @@
      }
    },

+    _handleTabChange() {
+      this._showMessagesView = this.$.commentTabs.selected === 0;
+    },
+
+    _computeShowMessages(showSection) {
+      return showSection ? 'visible' : '';
+    },
+
+    _computeShowThreads(showSection) {
+      return !showSection ? 'visible' : '';
+    },
+
    _handleEditCommitMessage(e) {
      this._editingCommitMessage = true;
      this.$.commitMessageEditor.focusTextarea();
@@ -603,6 +618,15 @@
      this._changeNum = value.changeNum;
      this.$.relatedChanges.clear();

+      // If the comment tabs were already rendered, but set to the wrong initial
+      // value, swap them here so the thread tab doesn't flash before being
+      // swapped out. If the selected tab is undefined, we have to wait until
+      // the page is finished rendering to set selected to 0, otherwise the
+      // animation will not show.
+      if (this.$.commentTabs.selected === 1) {
+        this.$.commentTabs.selected = 0;
+      }
+
      this._reload().then(() => {
        this._performPostLoadTasks();
      });
@@ -623,6 +647,10 @@

      this._sendShowChangeEvent();

+      // Selected has to be set after the paper-tabs are visible because
+      // the selected underline depends on calculations made by the browser.
+      this.$.commentTabs.selected = 0;
+
      this.async(() => {
        if (this.viewState.scrollTop) {
          document.documentElement.scrollTop =
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -327,11 +327,69 @@ limitations under the License.
    test('thread list modified', () => {
      sandbox.spy(element, '_handleReloadDiffComments');
      return element._reloadComments().then(() => {
-        element.fire('thread-list-modified');
+        element.$.threadList.fire('thread-list-modified');
        assert.isTrue(element._handleReloadDiffComments.called);
      });
    });

+    test('thread list and change log tabs', done => {
+      element._changeNum = '1';
+      element._patchRange = {
+        basePatchNum: 'PARENT',
+        patchNum: 1,
+      };
+      element._change = {
+        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
+        revisions: {
+          rev2: {_number: 2, commit: {parents: []}},
+          rev1: {_number: 1, commit: {parents: []}},
+          rev13: {_number: 13, commit: {parents: []}},
+          rev3: {_number: 3, commit: {parents: []}},
+        },
+        current_revision: 'rev3',
+        status: 'NEW',
+        labels: {
+          test: {
+            all: [],
+            default_value: 0,
+            values: [],
+            approved: {},
+          },
+        },
+      };
+      sandbox.stub(element.$.relatedChanges, 'reload');
+      sandbox.stub(element, '_reload').returns(Promise.resolve());
+      sandbox.spy(element, '_paramsChanged');
+      element.params = {view: 'change', changeNum: '1'};
+
+      // When the change is hard reloaded, paramsChanged will not set the tab.
+      // It will be set in postLoadTasks, which requires the flush() to detect.
+      assert.isTrue(element._paramsChanged.called);
+      assert.isUndefined(element.$.commentTabs.selected);
+
+      // Wait for tab to get selected
+      flush(() => {
+        assert.equal(element.$.commentTabs.selected, 0);
+        assert.notEqual(getComputedStyle(element.$.messageList).display,
+            'none');
+        assert.equal(getComputedStyle(element.$.threadList).display, 'none');
+
+        // Switch to comment thread tab
+        MockInteractions.tap(element.$$('paper-tab.commentThreads'));
+        assert.equal(element.$.commentTabs.selected, 1);
+        assert.equal(getComputedStyle(element.$.messageList).display,
+            'none');
+        assert.notEqual(getComputedStyle(element.$.threadList).display,
+            'none');
+
+        // When the change is partially reloaded (ex: Shift+R), paramsChanged
+        // will set the tab, if it was previously set to the thread tab.
+        element._paramsChanged(element.params);
+        assert.equal(element.$.commentTabs.selected, 0);
+        done();
+      });
+    });
+
    test('reply button is not visible when logged out', () => {
      assert.equal(getComputedStyle(element.$.replyBtn).display, 'none');
      element._loggedIn = true;
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.html
@@ -26,7 +26,8 @@ limitations under the License.
    <style include="shared-styles">
      :host,
      .messageListControls {
-        display: block;
+        display: flex;
+        justify-content: space-between;
      }
      .header {
        align-items: center;
@@ -64,8 +65,6 @@ limitations under the License.
      }
    </style>
    <div class="header">
-      <h3>Messages</h3>
-      <div class="messageListControls container">
        <span
            id="automatedMessageToggleContainer"
            class="container"
@@ -82,7 +81,6 @@ limitations under the License.
          [[_computeExpandCollapseMessage(_expanded)]]
        </gr-button>
      </div>
-    </div>
    <span
        id="messageControlsContainer"
        hidden$="[[_computeShowHideTextHidden(_visibleMessages, _processedMessages, _hideAutomated, _visibleMessages.length)]]">
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.js
@@ -92,8 +92,7 @@

    _handleCommentsChanged(e) {
      this.dispatchEvent(new CustomEvent('thread-list-modified',
-          {detail: {rootId: e.detail.rootId, path: e.detail.path},
-            bubbles: true}));
+          {detail: {rootId: e.detail.rootId, path: e.detail.path}}));
    },

    _isOnParent(side) {