Improve the UX of the attention set section in the reply dialog

Before: https://imgur.com/a/WZBcxWO After: https://imgur.com/a/V9GSC71 - Add icons that link to documentation and bug template. - Add a bug icon. - List the names of users that will be added to the attention set. - Make the selection chips nicer. - Blue border and background. Change border-radius. - Remove 'blurred' concept. - Move the "click chips ..." hint to the bottom. - Hide CC row, if there are no CCs. - Move overflow/linebreak/width rules from account-link to -label. - Fixes layout problems with many reviewer chips. Change-Id: Id4147eeddae2f6a137517ff8ebabb9eff34ca031
2020-07-24 09:55:54 +02:00
parent 63ce401262
commit 91675d717c
7 changed files with 216 additions and 85 deletions
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
@@ -39,6 +39,7 @@ import {SpecialFilePath} from '../../../constants/constants.js';
 import {ExperimentIds} from '../../../services/flags.js';
 import {fetchChangeUpdates} from '../../../utils/patch-set-util.js';
 import {KeyboardShortcutMixin} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.js';
+import {getDisplayName} from '../../../utils/display-name-util.js';

 const STORAGE_DEBOUNCE_INTERVAL_MS = 400;

@@ -773,6 +774,10 @@ class GrReplyDialog extends KeyboardShortcutMixin(GestureEventListeners(

  _handleAttentionModify() {
    this._attentionModified = true;
+    // If the attention-detail section is expanded without dispatching this
+    // event, then the dialog may expand beyond the screen's bottom border.
+    this.dispatchEvent(new CustomEvent(
+        'iron-resize', {composed: true, bubbles: true}));
  }

  _showAttentionSummary(config, attentionModified) {
@@ -813,6 +818,8 @@ class GrReplyDialog extends KeyboardShortcutMixin(GestureEventListeners(
            .map(id => parseInt(id)));
    const newAttention = new Set(this._currentAttentionSet);
    if (this._isOwner(user, change)) {
+      // TODO(brohlfs): Do not add all reviewers, just the ones that are replied
+      // to.
      reviewers.forEach(r => newAttention.add(r._account_id));
    } else {
      if (change.owner) {
@@ -823,6 +830,34 @@ class GrReplyDialog extends KeyboardShortcutMixin(GestureEventListeners(
    this._newAttentionSet = newAttention;
  }

+  _isNewAttentionEmpty(config, currentAttentionSet, newAttentionSet) {
+    return this._computeNewAttentionNames(
+        config, currentAttentionSet, newAttentionSet).length === 0;
+  }
+
+  _computeNewAttentionNames(config, currentAttentionSet, newAttentionSet) {
+    if ([currentAttentionSet, newAttentionSet].includes(undefined)) return '';
+    const addedNames = [...newAttentionSet]
+        .filter(id => !currentAttentionSet.has(id))
+        .map(id => this._findAccountById(id))
+        .filter(account => !!account)
+        .map(account => getDisplayName(config, account))
+        .sort();
+    return addedNames.join(', ');
+  }
+
+  _findAccountById(accountId) {
+    let allAccounts = [];
+    if (this.change && this.change.owner) allAccounts.push(this.change.owner);
+    if (this._reviewers) allAccounts = [...allAccounts, ...this._reviewers];
+    if (this._ccs) allAccounts = [...allAccounts, ...this._ccs];
+    return allAccounts.find(r => r._account_id === accountId);
+  }
+
+  _computeShowAttentionCcs(ccs) {
+    return !!ccs && ccs.length > 0;
+  }
+
  _accountOrGroupKey(entry) {
    return entry.id || entry._account_id;
  }
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
@@ -150,15 +150,27 @@ export const htmlTemplate = html`
    .attention .edit-attention-button iron-icon {
      color: inherit;
    }
+    .attention a,
+    .attention-detail a {
+      text-decoration: none;
+    }
+    .attentionSummary {
+      display: flex;
+      justify-content: space-between;
+    }
    .attention-detail .peopleList {
      margin-top: var(--spacing-s);
    }
+    .attention-detail .peopleList .accountList {
+      display: flex;
+      flex-wrap: wrap;
+    }
    .attention-detail gr-account-label {
-      background-color: var(--background-color-tertiary);
-      padding: 0 var(--spacing-m) 0 var(--spacing-s);
+      display: inline-block;
+      padding: var(--spacing-xs) var(--spacing-m);
      margin-right: var(--spacing-m);
      user-select: none;
-      --label-border-radius: 10px;
+      --label-border-radius: 4px;
    }
    .attention-detail gr-account-label:focus {
      outline: none;
@@ -168,7 +180,14 @@ export const htmlTemplate = html`
      cursor: pointer;
    }
    .attention-detail .attentionDetailsTitle {
-      margin-bottom: var(--spacing-s);
+      display: flex;
+      justify-content: space-between;
+      margin-bottom: var(--spacing-m);
+    }
+    .attention-detail .attentionDetailsFooter {
+      display: flex;
+      justify-content: space-between;
+      margin-top: var(--spacing-s);
    }
    .attention-detail .selectUsers {
      color: var(--deemphasized-text-color);
@@ -289,29 +308,60 @@ export const htmlTemplate = html`
      hidden$="[[!_showAttentionSummary(serverConfig, _attentionModified)]]"
      class="attention"
    >
-      <div>
-        <iron-icon class="attention-icon" icon="gr-icons:attention"></iron-icon>
-        <span hidden$="[[_isOwner(_account, change)]]"
-          >Bring to owner's attention.</span
-        >
-        <span hidden$="[[!_isOwner(_account, change)]]"
-          >Bring to all reviewer's attention.</span
-        >
-        <gr-button
-          class="edit-attention-button"
-          on-click="_handleAttentionModify"
-          link=""
-          position-below=""
-          data-label="Edit"
-          data-action-type="change"
-          data-action-key="edit"
-          title="Edit attention set changes"
-          role="button"
-          tabindex="0"
-        >
-          <iron-icon icon="gr-icons:edit" class=""></iron-icon>
-          Modify
-        </gr-button>
+      <div class="attentionSummary">
+        <div>
+          <iron-icon
+            class="attention-icon"
+            icon="gr-icons:attention"
+          ></iron-icon>
+          <template
+            is="dom-if"
+            if="[[_isNewAttentionEmpty(serverConfig, _currentAttentionSet, _newAttentionSet)]]"
+          >
+            <span>Do not add anyone to the attention set.</span>
+          </template>
+          <template
+            is="dom-if"
+            if="[[!_isNewAttentionEmpty(serverConfig, _currentAttentionSet, _newAttentionSet)]]"
+          >
+            <span
+              >Bring to attention of [[_computeNewAttentionNames(serverConfig,
+              _currentAttentionSet, _newAttentionSet)]].</span
+            >
+          </template>
+          <gr-button
+            class="edit-attention-button"
+            on-click="_handleAttentionModify"
+            link=""
+            position-below=""
+            data-label="Edit"
+            data-action-type="change"
+            data-action-key="edit"
+            title="Edit attention set changes"
+            role="button"
+            tabindex="0"
+          >
+            <iron-icon icon="gr-icons:edit"></iron-icon>
+            Modify
+          </gr-button>
+        </div>
+        <div>
+          <a
+            href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
+            target="_blank"
+          >
+            <iron-icon
+              icon="gr-icons:info"
+              title="read documentation"
+            ></iron-icon>
+          </a>
+          <a
+            href="https://bugs.chromium.org/p/gerrit/issues/entry?template=Attention+Set"
+            target="_blank"
+          >
+            <iron-icon icon="gr-icons:bug" title="report a problem"></iron-icon>
+          </a>
+        </div>
      </div>
    </section>
    <section
@@ -319,46 +369,82 @@ export const htmlTemplate = html`
      class="attention-detail"
    >
      <div class="attentionDetailsTitle">
-        <iron-icon class="attention-icon" icon="gr-icons:attention"></iron-icon>
-        <span>Bring to attention of ...</span>
-        <span class="selectUsers">(click chips to select users)</span>
+        <div>
+          <span>Change attention set to:</span>
+        </div>
+        <div></div>
+        <div>
+          <a
+            href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
+            target="_blank"
+          >
+            <iron-icon
+              icon="gr-icons:info"
+              title="read documentation"
+            ></iron-icon>
+          </a>
+          <a
+            href="https://bugs.chromium.org/p/gerrit/issues/entry?template=Attention+Set"
+            target="_blank"
+          >
+            <iron-icon icon="gr-icons:bug" title="report a problem"></iron-icon>
+          </a>
+        </div>
      </div>
      <div class="peopleList">
        <div class="peopleListLabel">Owner</div>
-        <gr-account-label
-          account="[[_owner]]"
-          force-attention="[[_computeHasNewAttention(_owner, _newAttentionSet)]]"
-          blurred="[[!_computeHasNewAttention(_owner, _newAttentionSet)]]"
-          hide-hovercard=""
-          on-click="_handleAttentionClick"
-        >
-        </gr-account-label>
+        <div>
+          <gr-account-label
+            account="[[_owner]]"
+            force-attention="[[_computeHasNewAttention(_owner, _newAttentionSet)]]"
+            selected$="[[_computeHasNewAttention(_owner, _newAttentionSet)]]"
+            deselected$="[[!_computeHasNewAttention(_owner, _newAttentionSet)]]"
+            hide-hovercard=""
+            on-click="_handleAttentionClick"
+          >
+          </gr-account-label>
+        </div>
      </div>
      <div class="peopleList">
        <div class="peopleListLabel">Reviewers</div>
-        <template is="dom-repeat" items="[[_reviewers]]" as="account">
-          <gr-account-label
-            account="[[account]]"
-            force-attention="[[_computeHasNewAttention(account, _newAttentionSet)]]"
-            blurred="[[!_computeHasNewAttention(account, _newAttentionSet)]]"
-            hide-hovercard=""
-            on-click="_handleAttentionClick"
-          >
-          </gr-account-label>
-        </template>
+        <div>
+          <template is="dom-repeat" items="[[_reviewers]]" as="account">
+            <gr-account-label
+              account="[[account]]"
+              force-attention="[[_computeHasNewAttention(account, _newAttentionSet)]]"
+              selected$="[[_computeHasNewAttention(account, _newAttentionSet)]]"
+              deselected$="[[!_computeHasNewAttention(account, _newAttentionSet)]]"
+              hide-hovercard=""
+              on-click="_handleAttentionClick"
+            >
+            </gr-account-label>
+          </template>
+        </div>
      </div>
-      <div class="peopleList">
-        <div class="peopleListLabel">CC</div>
-        <template is="dom-repeat" items="[[_ccs]]" as="account">
-          <gr-account-label
-            account="[[account]]"
-            force-attention="[[_computeHasNewAttention(account, _newAttentionSet)]]"
-            blurred="[[!_computeHasNewAttention(account, _newAttentionSet)]]"
-            hide-hovercard=""
-            on-click="_handleAttentionClick"
-          >
-          </gr-account-label>
-        </template>
+      <template is="dom-if" if="[[_computeShowAttentionCcs(_ccs)]]">
+        <div class="peopleList">
+          <div class="peopleListLabel">CC</div>
+          <div>
+            <template is="dom-repeat" items="[[_ccs]]" as="account">
+              <gr-account-label
+                account="[[account]]"
+                force-attention="[[_computeHasNewAttention(account, _newAttentionSet)]]"
+                selected$="[[_computeHasNewAttention(account, _newAttentionSet)]]"
+                deselected$="[[!_computeHasNewAttention(account, _newAttentionSet)]]"
+                hide-hovercard=""
+                on-click="_handleAttentionClick"
+              >
+              </gr-account-label>
+            </template>
+          </div>
+        </div>
+      </template>
+      <div class="attentionDetailsFooter">
+        <div></div>
+        <div>
+          <span class="selectUsers">(click chips to add and remove users)</span>
+        </div>
+        <div></div>
      </div>
    </section>
    <section
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
@@ -74,6 +74,7 @@ suite('gr-reply-dialog tests', () => {
      _number: changeNum,
      owner: {
        _account_id: 999,
+        display_name: 'Kermit',
      },
      labels: {
        'Verified': {
@@ -226,6 +227,25 @@ suite('gr-reply-dialog tests', () => {
    checkComputeAttention(1, [22, 33], 1, [22, 33], [22, 33]);
  });

+  test('computeNewAttentionNames', () => {
+    element._reviewers = [
+      {_account_id: 123, display_name: 'Ernie'},
+      {_account_id: 321, display_name: 'Bert'},
+    ];
+    element._ccs = [
+      {_account_id: 7, display_name: 'Elmo'},
+    ];
+    const compute = (currentAtt, newAtt) => element._computeNewAttentionNames(
+        undefined, new Set(currentAtt), new Set(newAtt));
+
+    assert.equal(compute([], []), '');
+    assert.equal(compute([], [999]), 'Kermit');
+    assert.equal(compute([999], []), '');
+    assert.equal(compute([999], [999]), '');
+    assert.equal(compute([123, 321], [999]), 'Kermit');
+    assert.equal(compute([999], [7, 123, 999]), 'Elmo, Ernie');
+  });
+
  test('toggle resolved checkbox', done => {
    // Async tick is needed because iron-selector content is distributed and
    // distributed content requires an observer to be set up.
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.js
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.js
@@ -66,11 +66,6 @@ class GrAccountLabel extends GestureEventListeners(
        type: Boolean,
        value: false,
      },
-      blurred: {
-        type: Boolean,
-        value: false,
-        reflectToAttribute: true,
-      },
      hideHovercard: {
        type: Boolean,
        value: false,
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label_html.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label_html.ts
@@ -19,25 +19,26 @@ import {html} from '@polymer/polymer/lib/utils/html-tag';
 export const htmlTemplate = html`
  <style include="shared-styles">
    :host {
-      display: inline;
      position: relative;
      border-radius: var(--label-border-radius);
+      /* Setting this really high, so all the following rules don't change
+           anything, only if --account-max-length is actually set to something
+           smaller like 20ch. */
+      max-width: var(--account-max-length, 500px);
+      white-space: nowrap;
+      overflow: hidden;
+      text-overflow: ellipsis;
    }
    :host::after {
      content: var(--account-label-suffix);
    }
-    :host(:not([blurred])) .overlay {
-      display: none;
+    :host([deselected]) {
+      background-color: white;
+      border: 1px solid #ddd;
    }
-    .overlay {
-      position: absolute;
-      pointer-events: none;
-      height: var(--line-height-normal);
-      right: 0;
-      left: 0;
-      background-color: var(--background-color-primary);
-      opacity: 0.5;
-      border-radius: var(--label-border-radius);
+    :host([selected]) {
+      background-color: #e8f0fe;
+      border: 1px solid #174ea6;
    }
    gr-avatar {
      height: var(--line-height-normal);
@@ -65,7 +66,6 @@ export const htmlTemplate = html`
      top: 2px;
    }
  </style>
-  <div class="overlay"></div>
  <span>
    <template is="dom-if" if="[[!hideHovercard]]">
      <gr-hovercard-account
--- a/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_html.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-link/gr-account-link_html.ts
@@ -20,14 +20,7 @@ export const htmlTemplate = html`
  <style include="shared-styles">
    :host {
      display: inline-block;
-      /* Setting this really high, so all the following rules don't change
-           anything, only if --account-max-length is actually set to something
-           smaller like 20ch. */
-      max-width: var(--account-max-length, 500px);
-      overflow: hidden;
-      text-overflow: ellipsis;
      vertical-align: top;
-      white-space: nowrap;
    }
    a {
      color: var(--primary-text-color);
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
@@ -106,6 +106,8 @@ $_documentContainer.innerHTML = `<iron-iconset-svg name="gr-icons" size="24">
      <g id="ready"><path d="M0 0h24v24H0z" fill="none"/><path d="M12 4.5C7 4.5 2.73 7.61 1 12c1.73 4.39 6 7.5 11 7.5s9.27-3.11 11-7.5c-1.73-4.39-6-7.5-11-7.5zM12 17c-2.76 0-5-2.24-5-5s2.24-5 5-5 5 2.24 5 5-2.24 5-5 5zm0-8c-1.66 0-3 1.34-3 3s1.34 3 3 3 3-1.34 3-3-1.34-3-3-3z"/></g>
      <!-- This SVG is a copy from iron-icons https://github.com/PolymerElements/iron-icons -->
      <g id="schedule"><path d="M11.99 2C6.47 2 2 6.48 2 12s4.47 10 9.99 10C17.52 22 22 17.52 22 12S17.52 2 11.99 2zM12 20c-4.42 0-8-3.58-8-8s3.58-8 8-8 8 3.58 8 8-3.58 8-8 8zm.5-13H11v6l5.25 3.15.75-1.23-4.5-2.67z"></path></g>
+      <!-- This SVG is a copy from material.io https://material.io/icons/#bug_report-->
+      <g id="bug"><path d="M0 0h24v24H0z" fill="none"/><path d="M20 8h-2.81c-.45-.78-1.07-1.45-1.82-1.96L17 4.41 15.59 3l-2.17 2.17C12.96 5.06 12.49 5 12 5c-.49 0-.96.06-1.41.17L8.41 3 7 4.41l1.62 1.63C7.88 6.55 7.26 7.22 6.81 8H4v2h2.09c-.05.33-.09.66-.09 1v1H4v2h2v1c0 .34.04.67.09 1H4v2h2.81c1.04 1.79 2.97 3 5.19 3s4.15-1.21 5.19-3H20v-2h-2.09c.05-.33.09-.66.09-1v-1h2v-2h-2v-1c0-.34-.04-.67-.09-1H20V8zm-6 8h-4v-2h4v2zm0-4h-4v-2h4v2z"/></g>
    </defs>
  </svg>
 </iron-iconset-svg>`;