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', () => {