From bcf84d2ea42470147ee66e52de6ac3ea0ffe299f Mon Sep 17 00:00:00 2001
From: Dmitrii Filippov <dmfilippov@google.com>
Date: Wed, 18 Mar 2020 15:31:59 +0100
Subject: [PATCH] Show reviewers next to each other

This affects only <gr-reviewer-list>, which is only used in
<gr-change-metadata>.

The goal is show more reviewers using the same amount of space. With the
new ability to show first names only two reviewers easily fit on one
row. So the metadata will consume less vertical space, or we can show
more reviewers using the same space.

Example screenshots:
Before: https://imgur.com/a/FmvUk6L
After: https://imgur.com/a/CHlM5vo

Change-Id: I23dd2080a7f0526f2d258f92c0bf8b6d356ea05b
---
 .../gr-change-metadata_html.js                |  7 +-
 .../gr-reviewer-list/gr-reviewer-list.js      | 17 ++--
 .../gr-reviewer-list/gr-reviewer-list_html.js | 25 +++---
 .../gr-reviewer-list_test.html                | 84 +++++++------------
 .../gr-change-metadata-shared-styles.js       |  5 +-
 5 files changed, 58 insertions(+), 80 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
index 786a118c7a..4d3af5b41d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
@@ -97,6 +97,9 @@ export const htmlTemplate = html`
       .topic gr-linked-chip {
         --linked-chip-text-color: var(--link-color);
       }
+      gr-reviewer-list {
+        max-width: 200px;
+      }
     </style>
     <gr-external-style id="externalStyle" name="change-metadata">
       <section>
@@ -145,13 +148,13 @@ export const htmlTemplate = html`
       <section>
         <span class="title">Reviewers</span>
         <span class="value">
-          <gr-reviewer-list change="{{change}}" mutable="[[_mutable]]" reviewers-only="" max-reviewers-displayed="3"></gr-reviewer-list>
+          <gr-reviewer-list change="{{change}}" mutable="[[_mutable]]" reviewers-only="" server-config="[[serverConfig]]"></gr-reviewer-list>
         </span>
       </section>
       <section>
         <span class="title">CC</span>
         <span class="value">
-          <gr-reviewer-list change="{{change}}" mutable="[[_mutable]]" ccs-only="" max-reviewers-displayed="3"></gr-reviewer-list>
+          <gr-reviewer-list change="{{change}}" mutable="[[_mutable]]" ccs-only="" server-config="[[serverConfig]]"></gr-reviewer-list>
         </span>
       </section>
       <template is="dom-if" if="[[_computeShowRepoBranchTogether(change.project, change.branch)]]">
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.js b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.js
index 129cb01c16..0fcf81deb4 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.js
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.js
@@ -44,6 +44,7 @@ class GrReviewerList extends GestureEventListeners(
   static get properties() {
     return {
       change: Object,
+      serverConfig: Object,
       disabled: {
         type: Boolean,
         value: false,
@@ -61,7 +62,6 @@ class GrReviewerList extends GestureEventListeners(
         type: Boolean,
         value: false,
       },
-      maxReviewersDisplayed: Number,
 
       _displayedReviewers: {
         type: Array,
@@ -92,7 +92,7 @@ class GrReviewerList extends GestureEventListeners(
 
   static get observers() {
     return [
-      '_reviewersChanged(change.reviewers.*, change.owner)',
+      '_reviewersChanged(change.reviewers.*, change.owner, serverConfig)',
     ];
   }
 
@@ -179,9 +179,9 @@ class GrReviewerList extends GestureEventListeners(
     return maxScores.join(', ');
   }
 
-  _reviewersChanged(changeRecord, owner) {
+  _reviewersChanged(changeRecord, owner, serverConfig) {
     // Polymer 2: check for undefined
-    if ([changeRecord, owner].some(arg => arg === undefined)) {
+    if ([changeRecord, owner, serverConfig].some(arg => arg === undefined)) {
       return;
     }
 
@@ -201,12 +201,13 @@ class GrReviewerList extends GestureEventListeners(
     this._reviewers = result
         .filter(reviewer => reviewer._account_id != owner._account_id);
 
+    const isFirstNameConfigured = serverConfig.accounts
+        && serverConfig.accounts.default_display_name === 'FIRST_NAME';
+    const maxReviewers = isFirstNameConfigured ? 6 : 3;
     // If there is one or two more than the max reviewers, don't show the
     // 'show more' button, because it takes up just as much space.
-    if (this.maxReviewersDisplayed &&
-        this._reviewers.length > this.maxReviewersDisplayed + 2) {
-      this._displayedReviewers =
-        this._reviewers.slice(0, this.maxReviewersDisplayed);
+    if (this._reviewers.length > maxReviewers + 2) {
+      this._displayedReviewers = this._reviewers.slice(0, maxReviewers);
     } else {
       this._displayedReviewers = this._reviewers;
     }
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_html.js b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_html.js
index c5df61d2df..300e38aa16 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_html.js
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_html.js
@@ -27,26 +27,27 @@ export const htmlTemplate = html`
       }
       .container {
         display: block;
-        /* This is a bit of a hack. We tried to use margin-top with
-           :not(:first-child) before, but :first-child does not understand
-           whether a child is visible or not. So adding a margin for every
-           child and then a negative one at the top does the trick. */
-        margin-top: calc(0px - var(--spacing-s));
-      }
-      .container > * {
-        margin-top: var(--spacing-s);
       }
       gr-button {
         --gr-button: {
           padding: 0px 0px;
         }
       }
+      gr-account-chip {
+        display: inline-block;
+      }
     </style>
     <div class="container">
-      <template is="dom-repeat" items="[[_displayedReviewers]]" as="reviewer">
-        <gr-account-chip class="reviewer" account="[[reviewer]]" on-remove="_handleRemove" voteable-text="[[_computeVoteableText(reviewer, change)]]" removable="[[_computeCanRemoveReviewer(reviewer, mutable)]]">
-        </gr-account-chip>
-      </template>
+      <div>
+        <template is="dom-repeat" items="[[_displayedReviewers]]" as="reviewer">
+          <gr-account-chip class="reviewer"
+                           account="[[reviewer]]"
+                           on-remove="_handleRemove"
+                           voteable-text="[[_computeVoteableText(reviewer, change)]]"
+                           removable="[[_computeCanRemoveReviewer(reviewer, mutable)]]">
+          </gr-account-chip>
+        </template>
+      </div>
       <gr-button class="hiddenReviewers" link="" hidden\$="[[!_hiddenReviewerCount]]" on-click="_handleViewAll">and [[_hiddenReviewerCount]] more</gr-button>
       <div class="controlsContainer" hidden\$="[[!mutable]]">
         <gr-button link="" id="addReviewer" class="addReviewer" on-click="_handleAddTap">[[_addLabel]]</gr-button>
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.html b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.html
index d97bf6b0a1..9e51d1688f 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.html
@@ -40,6 +40,7 @@ suite('gr-reviewer-list tests', () => {
 
   setup(() => {
     element = fixture('basic');
+    element.serverConfig = {};
     sandbox = sinon.sandbox.create();
     stub('gr-rest-api-interface', {
       getConfig() { return Promise.resolve({}); },
@@ -198,39 +199,38 @@ suite('gr-reviewer-list tests', () => {
         {value: {ccsOnly: true}});
   });
 
-  test('no show all reviewers button with 6 reviewers', () => {
+  test('dont show all reviewers button with 4 reviewers', () => {
+    const reviewers = [];
+    element.maxReviewersDisplayed = 3;
+    for (let i = 0; i < 4; i++) {
+      reviewers.push(
+          {email: i+'reviewer@google.com', name: 'reviewer-' + i});
+    }
+    element.ccsOnly = true;
+
+    element.change = {
+      owner: {
+        _account_id: 1,
+      },
+      reviewers: {
+        CC: reviewers,
+      },
+    };
+    assert.equal(element._hiddenReviewerCount, 0);
+    assert.equal(element._displayedReviewers.length, 4);
+    assert.equal(element._reviewers.length, 4);
+    assert.isTrue(element.shadowRoot
+        .querySelector('.hiddenReviewers').hidden);
+  });
+
+  test('show all reviewers button with 6 reviewers', () => {
     const reviewers = [];
-    element.maxReviewersDisplayed = 5;
     for (let i = 0; i < 6; i++) {
       reviewers.push(
           {email: i+'reviewer@google.com', name: 'reviewer-' + i});
     }
     element.ccsOnly = true;
 
-    element.change = {
-      owner: {
-        _account_id: 1,
-      },
-      reviewers: {
-        CC: reviewers,
-      },
-    };
-    assert.equal(element._hiddenReviewerCount, 0);
-    assert.equal(element._displayedReviewers.length, 6);
-    assert.equal(element._reviewers.length, 6);
-    assert.isTrue(element.shadowRoot
-        .querySelector('.hiddenReviewers').hidden);
-  });
-
-  test('show all reviewers button with 8 reviewers', () => {
-    const reviewers = [];
-    element.maxReviewersDisplayed = 5;
-    for (let i = 0; i < 8; i++) {
-      reviewers.push(
-          {email: i+'reviewer@google.com', name: 'reviewer-' + i});
-    }
-    element.ccsOnly = true;
-
     element.change = {
       owner: {
         _account_id: 1,
@@ -240,38 +240,14 @@ suite('gr-reviewer-list tests', () => {
       },
     };
     assert.equal(element._hiddenReviewerCount, 3);
-    assert.equal(element._displayedReviewers.length, 5);
-    assert.equal(element._reviewers.length, 8);
+    assert.equal(element._displayedReviewers.length, 3);
+    assert.equal(element._reviewers.length, 6);
     assert.isFalse(element.shadowRoot
         .querySelector('.hiddenReviewers').hidden);
   });
 
-  test('no maxReviewersDisplayed', () => {
-    const reviewers = [];
-    for (let i = 0; i < 7; i++) {
-      reviewers.push(
-          {email: i+'reviewer@google.com', name: 'reviewer-' + i});
-    }
-    element.ccsOnly = true;
-
-    element.change = {
-      owner: {
-        _account_id: 1,
-      },
-      reviewers: {
-        CC: reviewers,
-      },
-    };
-    assert.equal(element._hiddenReviewerCount, 0);
-    assert.equal(element._displayedReviewers.length, 7);
-    assert.equal(element._reviewers.length, 7);
-    assert.isTrue(element.shadowRoot
-        .querySelector('.hiddenReviewers').hidden);
-  });
-
   test('show all reviewers button', () => {
     const reviewers = [];
-    element.maxReviewersDisplayed = 5;
     for (let i = 0; i < 100; i++) {
       reviewers.push(
           {email: i+'reviewer@google.com', name: 'reviewer-' + i});
@@ -286,8 +262,8 @@ suite('gr-reviewer-list tests', () => {
         CC: reviewers,
       },
     };
-    assert.equal(element._hiddenReviewerCount, 95);
-    assert.equal(element._displayedReviewers.length, 5);
+    assert.equal(element._hiddenReviewerCount, 97);
+    assert.equal(element._displayedReviewers.length, 3);
     assert.equal(element._reviewers.length, 100);
     assert.isFalse(element.shadowRoot
         .querySelector('.hiddenReviewers').hidden);
diff --git a/polygerrit-ui/app/styles/gr-change-metadata-shared-styles.js b/polygerrit-ui/app/styles/gr-change-metadata-shared-styles.js
index 51cf6d3759..aabdde58f6 100644
--- a/polygerrit-ui/app/styles/gr-change-metadata-shared-styles.js
+++ b/polygerrit-ui/app/styles/gr-change-metadata-shared-styles.js
@@ -34,6 +34,7 @@ $_documentContainer.innerHTML = `<dom-module id="gr-change-metadata-shared-style
       .title,
       .value {
         display: table-cell;
+        vertical-align: top;
       }
 
       .title {
@@ -43,10 +44,6 @@ $_documentContainer.innerHTML = `<dom-module id="gr-change-metadata-shared-style
         padding-right: var(--metadata-horizontal-padding);
         word-break: break-word;
       }
-
-      .value {
-        padding-right: var(--metadata-horizontal-padding);
-      }
     </style>
   </template>
 </dom-module>`;