Swap 'expand inline' and 'reviewed' controls
In the file list, the 'expand inline' and 'reviewed' controls should be
swapped. Contextually, it makes more sense for the 'expand' control to
appear first, as one needs no information about the file in order to
want to expand it -- in the case of the reviewed checkbox, the user is
expected to have viewed the file before toggling the checkbox state.
In addition, some work was done to migrate the reviewed checkbox to the
new button-like styling designed by Arnab.
Bug: Issue 7275
Change-Id: I5a052da8fdce79523709e9c98b01466cb26eb242
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 3464fea..10f4b27 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -37,6 +37,7 @@
display: block;
}
.row {
+ align-items: center;
border-top: 1px solid #eee;
display: flex;
padding: .1em .25em;
@@ -44,6 +45,9 @@
:host(.loading) .row {
opacity: .5;
};
+ :host(.editLoaded) .hideOnEdit {
+ display: none;
+ }
.reviewed,
.status {
align-items: center;
@@ -107,14 +111,13 @@
font-family: var(--font-family-bold);
}
.show-hide {
- margin-left: .4em;
+ width: 1em;
}
.fileListButton {
margin: .5em;
}
.totalChanges {
justify-content: flex-end;
- padding-right: 2.6em;
text-align: right;
}
.warning {
@@ -129,7 +132,6 @@
display: block;
font-size: .8em;
min-width: 2em;
- margin-top: .1em;
}
gr-diff {
box-shadow: 0 1px 3px rgba(0, 0, 0, .3);
@@ -147,12 +149,39 @@
.mobile {
display: none;
}
- #container.editLoaded .hideOnEdit {
- display: none;
- }
.expandInline {
padding-right: .25em;
}
+ .reviewed {
+ margin-left: 2em;
+ width: 15em;
+ }
+ .reviewed label {
+ color: #2A66D9;
+ opacity: 0;
+ justify-content: flex-end;
+ width: 100%;
+ }
+ .reviewed label:hover {
+ cursor: pointer;
+ opacity: 100;
+ }
+ .row:hover .reviewed label,
+ .row:focus .reviewed label {
+ opacity: 100;
+ }
+ .reviewed input {
+ display: none;
+ }
+ .reviewedLabel {
+ color: rgba(0, 0, 0, .54);
+ margin-right: 1em;
+ opacity: 0;
+ }
+ .reviewedLabel.isReviewed {
+ display: initial;
+ opacity: 100;
+ }
@media screen and (max-width: 50em) {
.desktop {
display: none;
@@ -185,7 +214,6 @@
</style>
<div
id="container"
- class$="[[_computeContainerClass(editLoaded)]]"
on-tap="_handleFileListTap">
<template is="dom-repeat"
items="[[_shownFiles]]"
@@ -194,13 +222,14 @@
initial-count="[[fileListIncrement]]"
target-framerate="1">
<div class="file-row row" data-path$="[[file.__path]]" tabindex="-1">
- <div class="reviewed hideOnEdit" hidden$="[[!_loggedIn]]" hidden>
- <input
- type="checkbox"
- checked="[[file.isReviewed]]"
- class="reviewed"
- aria-label="Reviewed checkbox"
- title="Mark as reviewed (shortcut: r)">
+ <div class="show-hide" hidden$="[[_userPrefs.expand_inline_diffs]]">
+ <label class="show-hide" data-path$="[[file.__path]]"
+ data-expand=true>
+ <input type="checkbox" class="show-hide"
+ checked$="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]"
+ data-path$="[[file.__path]]" data-expand=true>
+ [[_computeShowHideText(file.__path, _expandedFilePaths.*)]]
+ </label>
</div>
<div class$="[[_computeClass('status', file.__path)]]"
tabindex="0"
@@ -261,13 +290,11 @@
[[_formatPercentage(file.size, file.size_delta)]]
</span>
</div>
- <div class="show-hide" hidden$="[[_userPrefs.expand_inline_diffs]]">
- <label class="show-hide" data-path$="[[file.__path]]"
- data-expand=true>
- <input type="checkbox" class="show-hide"
- checked$="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]"
- data-path$="[[file.__path]]" data-expand=true>
- [[_computeShowHideText(file.__path, _expandedFilePaths.*)]]
+ <div class="reviewed hideOnEdit" hidden$="[[!_loggedIn]]" hidden>
+ <span class$="reviewedLabel [[_computeReviewedClass(file.isReviewed)]]">Reviewed</span>
+ <label>
+ <input class="reviewed" type="checkbox" checked="[[file.isReviewed]]">
+ <span class="markReviewed" title="Mark as reviewed (shortcut: r)">[[_computeReviewedText(file.isReviewed)]]</span>
</label>
</div>
</div>
@@ -307,6 +334,8 @@
-[[_patchChange.deleted]]
</span>
</div>
+ <!-- Empty div here exists to keep spacing in sync with file rows. -->
+ <div class="reviewed hideOnEdit" hidden$="[[!_loggedIn]]" hidden></div>
</div>
<div
class$="row totalChanges [[_computeExpandInlineClass(_userPrefs)]]"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index 100bdee..7807f5e 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -59,7 +59,10 @@
notify: true,
observer: '_updateDiffPreferences',
},
- editLoaded: Boolean,
+ editLoaded: {
+ type: Boolean,
+ observer: '_editLoadedChanged',
+ },
_files: {
type: Array,
observer: '_filesChanged',
@@ -424,9 +427,8 @@
row = row.parentElement;
}
const path = row.dataset.path;
-
// Handle checkbox mark as reviewed.
- if (e.target.classList.contains('reviewed')) {
+ if (e.target.classList.contains('markReviewed')) {
return this._reviewFile(path);
}
@@ -720,7 +722,7 @@
},
_computeShowHideText(path, expandedFilesRecord) {
- return this._isFileExpanded(path, expandedFilesRecord) ? '▼' : '◀';
+ return this._isFileExpanded(path, expandedFilesRecord) ? '▼' : '▶';
},
_computeFilesShown(numFilesShown, files) {
@@ -910,8 +912,16 @@
}, LOADING_DEBOUNCE_INTERVAL);
},
- _computeContainerClass(editLoaded) {
- return editLoaded ? 'editLoaded' : '';
+ _editLoadedChanged(editLoaded) {
+ this.classList.toggle('editLoaded', editLoaded);
+ },
+
+ _computeReviewedClass(isReviewed) {
+ return isReviewed ? 'isReviewed' : '';
+ },
+
+ _computeReviewedText(isReviewed) {
+ return isReviewed ? 'MARK UNREVIEWED' : 'MARK REVIEWED';
},
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index 4b407c4..e77ad2c 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -609,21 +609,29 @@
flushAsynchronousOperations();
const fileRows =
Polymer.dom(element.root).querySelectorAll('.row:not(.header)');
- const commitMsg = fileRows[0].querySelector(
- 'input.reviewed[type="checkbox"]');
- const fileAdded = fileRows[1].querySelector(
- 'input.reviewed[type="checkbox"]');
- const myFile = fileRows[2].querySelector(
- 'input.reviewed[type="checkbox"]');
+ const checkSelector = 'input.reviewed[type="checkbox"]';
+ const commitMsg = fileRows[0].querySelector(checkSelector);
+ const fileAdded = fileRows[1].querySelector(checkSelector);
+ const myFile = fileRows[2].querySelector(checkSelector);
assert.isTrue(commitMsg.checked);
assert.isFalse(fileAdded.checked);
assert.isTrue(myFile.checked);
- MockInteractions.tap(commitMsg);
+ const commitReviewLabel = fileRows[0].querySelector('.reviewedLabel');
+ const markReviewLabel = commitMsg.nextElementSibling;
+ assert.isTrue(commitReviewLabel.classList.contains('isReviewed'));
+ assert.equal(markReviewLabel.textContent, 'MARK UNREVIEWED');
+
+ MockInteractions.tap(markReviewLabel);
assert.isTrue(saveStub.lastCall.calledWithExactly('/COMMIT_MSG', false));
- MockInteractions.tap(commitMsg);
+ assert.isFalse(commitReviewLabel.classList.contains('isReviewed'));
+ assert.equal(markReviewLabel.textContent, 'MARK REVIEWED');
+
+ MockInteractions.tap(markReviewLabel);
assert.isTrue(saveStub.lastCall.calledWithExactly('/COMMIT_MSG', true));
+ assert.isTrue(commitReviewLabel.classList.contains('isReviewed'));
+ assert.equal(markReviewLabel.textContent, 'MARK UNREVIEWED');
});
test('patch set from revisions', () => {
@@ -1207,12 +1215,15 @@
test('reviewed checkbox', () => {
const alertStub = sandbox.stub();
+ const saveReviewStub = sandbox.stub(element, '_saveReviewedState');
+
element.addEventListener('show-alert', alertStub);
element.editLoaded = false;
// Reviewed checkbox should be shown.
assert.isTrue(isVisible(element.$$('.reviewed')));
MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isFalse(alertStub.called);
+ assert.isTrue(saveReviewStub.calledOnce);
element.editLoaded = true;
flushAsynchronousOperations();
@@ -1220,6 +1231,7 @@
assert.isFalse(isVisible(element.$$('.reviewed')));
MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(alertStub.called);
+ assert.isTrue(saveReviewStub.calledOnce);
});
test('_getReviewedFiles does not call API', () => {
