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
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 786a118..4d3af5b 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 @@
.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 @@
<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 129cb01..0fcf81d 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 @@
static get properties() {
return {
change: Object,
+ serverConfig: Object,
disabled: {
type: Boolean,
value: false,
@@ -61,7 +62,6 @@
type: Boolean,
value: false,
},
- maxReviewersDisplayed: Number,
_displayedReviewers: {
type: Array,
@@ -92,7 +92,7 @@
static get observers() {
return [
- '_reviewersChanged(change.reviewers.*, change.owner)',
+ '_reviewersChanged(change.reviewers.*, change.owner, serverConfig)',
];
}
@@ -179,9 +179,9 @@
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 @@
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 c5df61d..300e38a 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 @@
}
.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 d97bf6b..9e51d16 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 @@
setup(() => {
element = fixture('basic');
+ element.serverConfig = {};
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getConfig() { return Promise.resolve({}); },
@@ -198,9 +199,32 @@
{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 = 5;
+ 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 = [];
for (let i = 0; i < 6; i++) {
reviewers.push(
{email: i+'reviewer@google.com', name: 'reviewer-' + i});
@@ -215,63 +239,15 @@
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,
- },
- reviewers: {
- CC: reviewers,
- },
- };
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 @@
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 51cf6d3..aabdde5 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 @@
.title,
.value {
display: table-cell;
+ vertical-align: top;
}
.title {
@@ -43,10 +44,6 @@
padding-right: var(--metadata-horizontal-padding);
word-break: break-word;
}
-
- .value {
- padding-right: var(--metadata-horizontal-padding);
- }
</style>
</template>
</dom-module>`;
