commit 2c15ec0698aa24c897883594223a24541c5b636b Author: Ben Rohlfs Date: Tue Sep 29 17:32:02 2020 +0200 Highlight the dashboard rows with attention as reviewer We are using the former assignee-highlight-color. And we are making sure that the row that is selected *and* highlighted is still looking highlighted, but with a slightly different shade of yellow. Change-Id: Iad9b6eb7d8f693529199ac4dbf11d9b541c5c6ca diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.ts index 497ed1e..2e98349 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_html.ts @@ -32,9 +32,6 @@ export const htmlTemplate = html` font-weight: var(--font-weight-bold); color: var(--primary-text-color); } - :host([highlight]) { - background-color: var(--assignee-highlight-color); - } .container { position: relative; } diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts index a05ccf4..974d648 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts @@ -37,10 +37,11 @@ import { import { GerritNav, DashboardSection, + YOUR_TURN, } from '../../core/gr-navigation/gr-navigation'; import {getPluginEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints'; import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader'; -import {changeIsOpen} from '../../../utils/change-util'; +import {changeIsOpen, isOwner} from '../../../utils/change-util'; import {customElement, property, observe} from '@polymer/decorators'; import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api'; import {GrCursorManager} from '../../shared/gr-cursor-manager/gr-cursor-manager'; @@ -50,6 +51,7 @@ import { ServerInfo, PreferencesInput, } from '../../../types/common'; +import {hasAttention, isAttentionSetEnabled} from '../../../utils/account-util'; const NUMBER_FIXED_COLUMNS = 3; const CLOSED_STATUS = ['MERGED', 'ABANDONED']; @@ -339,17 +341,19 @@ export class GrChangeList extends ChangeTableMixin( ); } - _computeItemHighlight(account?: AccountInfo, change?: ChangeInfo) { - // Do not show the assignee highlight if the change is not open. - if ( - !change || - !change.assignee || - !account || - CLOSED_STATUS.indexOf(change.status) !== -1 - ) { - return false; - } - return account._account_id === change.assignee._account_id; + _computeItemHighlight( + account?: AccountInfo, + change?: ChangeInfo, + config?: ServerInfo, + sectionName?: string + ) { + if (!change || !account) return false; + if (CLOSED_STATUS.indexOf(change.status) !== -1) return false; + return isAttentionSetEnabled(config) + ? hasAttention(config, account, change) && + !isOwner(change, account) && + sectionName === YOUR_TURN.name + : account._account_id === change.assignee?._account_id; } _nextChange(e: CustomKeyboardEvent) { @@ -492,7 +496,7 @@ export class GrChangeList extends ChangeTableMixin( _getSpecialEmptySlot(section: DashboardSection) { if (section.isOutgoing) return 'empty-outgoing'; - if (section.name === 'Your Turn') return 'empty-your-turn'; + if (section.name === YOUR_TURN.name) return 'empty-your-turn'; return ''; } diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts index f223fbf..eb489b0 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts @@ -139,7 +139,7 @@ export const htmlTemplate = html` { }); test('shown on empty outgoing sections', () => { - const section = {results: [], name: 'Your Turn'}; + const section = {results: [], name: YOUR_TURN.name}; assert.isTrue(element._isEmpty(section)); assert.equal(element._getSpecialEmptySlot(section), 'empty-your-turn'); }); diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts index db0bd64..7b56688 100644 --- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts +++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts @@ -147,80 +147,88 @@ export interface UserDashboard { // NOTE: These queries are tested in Java. Any changes made to definitions // here require corresponding changes to: // java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java +const HAS_DRAFTS: DashboardSection = { + // Changes with unpublished draft comments. This section is omitted when + // viewing other users, so we don't need to filter anything out. + name: 'Has draft comments', + query: 'has:draft', + selfOnly: true, + hideIfEmpty: true, + suffixForDashboard: 'limit:10', +}; +export const YOUR_TURN: DashboardSection = { + // Changes where the user is in the attention set. + name: 'Your Turn', + query: 'attention:${user}', + hideIfEmpty: false, + suffixForDashboard: 'limit:25', + attentionSetOnly: true, +}; +const ASSIGNED: DashboardSection = { + // Changes that are assigned to the viewed user. + name: 'Assigned reviews', + query: + 'assignee:${user} (-is:wip OR owner:self OR assignee:self) ' + + 'is:open -is:ignored', + hideIfEmpty: true, + suffixForDashboard: 'limit:25', + assigneeOnly: true, +}; +const WIP: DashboardSection = { + // WIP open changes owned by viewing user. This section is omitted when + // viewing other users, so we don't need to filter anything out. + name: 'Work in progress', + query: 'is:open owner:${user} is:wip', + selfOnly: true, + hideIfEmpty: true, + suffixForDashboard: 'limit:25', +}; +const OUTGOING: DashboardSection = { + // Non-WIP open changes owned by viewed user. Filter out changes ignored + // by the viewing user. + name: 'Outgoing reviews', + query: 'is:open owner:${user} -is:wip -is:ignored', + isOutgoing: true, + suffixForDashboard: 'limit:25', +}; +const INCOMING: DashboardSection = { + // Non-WIP open changes not owned by the viewed user, that the viewed user + // is associated with (as either a reviewer or the assignee). Changes + // ignored by the viewing user are filtered out. + name: 'Incoming reviews', + query: + 'is:open -owner:${user} -is:wip -is:ignored ' + + '(reviewer:${user} OR assignee:${user})', + suffixForDashboard: 'limit:25', +}; +const CCED: DashboardSection = { + // Open changes the viewed user is CCed on. Changes ignored by the viewing + // user are filtered out. + name: 'CCed on', + query: 'is:open -is:ignored cc:${user}', + suffixForDashboard: 'limit:10', +}; +const CLOSED: DashboardSection = { + name: 'Recently closed', + // Closed changes where viewed user is owner, reviewer, or assignee. + // Changes ignored by the viewing user are filtered out, and so are WIP + // changes not owned by the viewing user (the one instance of + // 'owner:self' is intentional and implements this logic). + query: + 'is:closed -is:ignored (-is:wip OR owner:self) ' + + '(owner:${user} OR reviewer:${user} OR assignee:${user} ' + + 'OR cc:${user})', + suffixForDashboard: '-age:4w limit:10', +}; const DEFAULT_SECTIONS: DashboardSection[] = [ - { - // Changes with unpublished draft comments. This section is omitted when - // viewing other users, so we don't need to filter anything out. - name: 'Has draft comments', - query: 'has:draft', - selfOnly: true, - hideIfEmpty: true, - suffixForDashboard: 'limit:10', - }, - { - // Changes where the user is in the attention set. - name: 'Your Turn', - query: 'attention:${user}', - hideIfEmpty: false, - suffixForDashboard: 'limit:25', - attentionSetOnly: true, - }, - { - // Changes that are assigned to the viewed user. - name: 'Assigned reviews', - query: - 'assignee:${user} (-is:wip OR owner:self OR assignee:self) ' + - 'is:open -is:ignored', - hideIfEmpty: true, - suffixForDashboard: 'limit:25', - assigneeOnly: true, - }, - { - // WIP open changes owned by viewing user. This section is omitted when - // viewing other users, so we don't need to filter anything out. - name: 'Work in progress', - query: 'is:open owner:${user} is:wip', - selfOnly: true, - hideIfEmpty: true, - suffixForDashboard: 'limit:25', - }, - { - // Non-WIP open changes owned by viewed user. Filter out changes ignored - // by the viewing user. - name: 'Outgoing reviews', - query: 'is:open owner:${user} -is:wip -is:ignored', - isOutgoing: true, - suffixForDashboard: 'limit:25', - }, - { - // Non-WIP open changes not owned by the viewed user, that the viewed user - // is associated with (as either a reviewer or the assignee). Changes - // ignored by the viewing user are filtered out. - name: 'Incoming reviews', - query: - 'is:open -owner:${user} -is:wip -is:ignored ' + - '(reviewer:${user} OR assignee:${user})', - suffixForDashboard: 'limit:25', - }, - { - // Open changes the viewed user is CCed on. Changes ignored by the viewing - // user are filtered out. - name: 'CCed on', - query: 'is:open -is:ignored cc:${user}', - suffixForDashboard: 'limit:10', - }, - { - name: 'Recently closed', - // Closed changes where viewed user is owner, reviewer, or assignee. - // Changes ignored by the viewing user are filtered out, and so are WIP - // changes not owned by the viewing user (the one instance of - // 'owner:self' is intentional and implements this logic). - query: - 'is:closed -is:ignored (-is:wip OR owner:self) ' + - '(owner:${user} OR reviewer:${user} OR assignee:${user} ' + - 'OR cc:${user})', - suffixForDashboard: '-age:4w limit:10', - }, + HAS_DRAFTS, + YOUR_TURN, + ASSIGNED, + WIP, + OUTGOING, + INCOMING, + CCED, + CLOSED, ]; export interface GenerateUrlSearchViewParameters { diff --git a/polygerrit-ui/app/styles/gr-change-list-styles.ts b/polygerrit-ui/app/styles/gr-change-list-styles.ts index 1dbc917..38a94d4 100644 --- a/polygerrit-ui/app/styles/gr-change-list-styles.ts +++ b/polygerrit-ui/app/styles/gr-change-list-styles.ts @@ -32,6 +32,13 @@ $_documentContainer.innerHTML = ` gr-change-list-item:focus { background-color: var(--selection-background-color); } + gr-change-list-item[highlight] { + background-color: var(--assignee-highlight-color); + } + gr-change-list-item[highlight][selected], + gr-change-list-item[highlight]:focus { + background-color: var(--assignee-highlight-selection-color); + } .groupTitle td, .cell { vertical-align: middle; diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts index 835ef0a..2a3e91b 100644 --- a/polygerrit-ui/app/styles/themes/app-theme.ts +++ b/polygerrit-ui/app/styles/themes/app-theme.ts @@ -71,6 +71,9 @@ $_documentContainer.innerHTML = ` --view-background-color: var(--background-color-primary); /* unique background colors */ --assignee-highlight-color: #fcfad6; + /* TODO: Find a nicer way to combine the --assignee-highlight-color and the + --selection-background-color than to just invent another unique color. */ + --assignee-highlight-selection-color: #f6f4d0; --chip-selected-background-color: #e8f0fe; --edit-mode-background-color: #ebf5fb; --emphasis-color: #fff9c4; diff --git a/polygerrit-ui/app/styles/themes/dark-theme.ts b/polygerrit-ui/app/styles/themes/dark-theme.ts index 3032984..03a3ab5 100644 --- a/polygerrit-ui/app/styles/themes/dark-theme.ts +++ b/polygerrit-ui/app/styles/themes/dark-theme.ts @@ -55,6 +55,7 @@ function getStyleEl() { /* empty, because inheriting from app-theme is just fine /* unique background colors */ --assignee-highlight-color: #3a361c; + --assignee-highlight-selection-color: #423e24; --chip-selected-background-color: #3c4455; --edit-mode-background-color: #5c0a36; --emphasis-color: #383f4a; diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts index 4504ffd..9ad5648 100644 --- a/polygerrit-ui/app/utils/account-util.ts +++ b/polygerrit-ui/app/utils/account-util.ts @@ -38,23 +38,22 @@ export function removeServiceUsers(accounts?: AccountInfo[]): AccountInfo[] { return accounts?.filter(a => !isServiceUser(a)) || []; } -export function isAttentionSetEnabled(config: ServerInfo): boolean { +export function isAttentionSetEnabled(config?: ServerInfo): boolean { return !!config?.change?.enable_attention_set; } -export function canHaveAttention(account: AccountInfo): boolean { +export function canHaveAttention(account?: AccountInfo): boolean { return !!account && !!account._account_id && !isServiceUser(account); } export function hasAttention( - config: ServerInfo, - account: AccountInfo, - change: ChangeInfo + config?: ServerInfo, + account?: AccountInfo, + change?: ChangeInfo ): boolean { return ( isAttentionSetEnabled(config) && canHaveAttention(account) && - !!account._account_id && - !!change?.attention_set?.hasOwnProperty(account._account_id) + !!change?.attention_set?.hasOwnProperty(account?._account_id!) ); } diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts index 2c9d9c3..11c861e 100644 --- a/polygerrit-ui/app/utils/change-util.ts +++ b/polygerrit-ui/app/utils/change-util.ts @@ -18,6 +18,7 @@ import {getBaseUrl} from './url-util'; import {ChangeStatus} from '../constants/constants'; import {NumericChangeId, PatchSetNum, ChangeInfo} from '../types/common'; import {ParsedChangeInfo} from '../elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser'; +import {AccountInfo} from '../types/common'; // This can be wrong! See WARNING above interface ChangeStatusesOptions { @@ -167,6 +168,11 @@ export function changeStatuses( return states; } +export function isOwner(change?: ChangeInfo, account?: AccountInfo) { + if (!change || !account) return false; + return change.owner?._account_id === account._account_id; +} + export function changeStatusString(change: ChangeInfo) { return changeStatuses(change).join(', '); }