opendev/gerrit
Code Issues Proposed changes

Show reviewers next to each other

Browse Source 
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
This commit is contained in:
Dmitrii Filippov
2020-03-18 15:31:59 +01:00
committed by Ben Rohlfs
parent 9fcb519992
commit bcf84d2ea4
5 changed files with 58 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.js
View File

@@ -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)]]">

17
polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.js
View File

@@ -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;
}

25
polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_html.js
View File

@@ -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>

84
polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.html
View File

@@ -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);

5
polygerrit-ui/app/styles/gr-change-metadata-shared-styles.js
View File

@@ -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>`;