opendev/gerrit
Code Issues Proposed changes

Remove 'edit' button from file list rows

Browse Source 
- Clicking on file path when in edit mode now navigates to editor view
- 'Edit' action renamed to 'Open'
- 'More' menu renamed to 'Actions'

Bug: Issue 8333
Change-Id: I1dcbec6a742692d3d07fc6070fdd3dc72fabe77a
This commit is contained in:
Kasper Nilsson
2018-02-09 14:34:14 -08:00
parent ad49bcd420
commit 40d5f3fa6c
12 changed files with 65 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -1416,7 +1416,7 @@
case GrEditConstants.Actions.DELETE.id:
controls.openDeleteDialog(path);
break;
case GrEditConstants.Actions.EDIT.id:
case GrEditConstants.Actions.OPEN.id:
Gerrit.Nav.navigateToRelativeUrl(
Gerrit.Nav.getEditUrlForDiff(this._change, path,
this._patchRange.patchNum));

4
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -1439,9 +1439,9 @@ limitations under the License.
assert.isTrue(controls.openRenameDialog.called);
assert.equal(controls.openRenameDialog.lastCall.args[0], 'foo');
// Edit
// Open
fileList.dispatchEvent(new CustomEvent('file-action-tap',
{detail: {action: Actions.EDIT.id, path: 'foo'}, bubbles: true}));
{detail: {action: Actions.OPEN.id, path: 'foo'}, bubbles: true}));
flushAsynchronousOperations();
assert.isTrue(Gerrit.Nav.getEditUrlForDiff.called);

7
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
View File

@@ -225,8 +225,7 @@ limitations under the License.
opacity: 100;
}
.editFileControls {
margin-left: 1em;
width: 10em;
width: 7em;
}
@media screen and (max-width: 50em) {
.desktop {
@@ -279,9 +278,9 @@ limitations under the License.
[[_computeFileStatus(file.status)]]
</div>
<span
data-url="[[_computeDiffURL(change, patchRange.patchNum, patchRange.basePatchNum, file.__path)]]"
data-url="[[_computeDiffURL(change, patchRange.patchNum, patchRange.basePatchNum, file.__path, editMode)]]"
class="path">
<a class="pathLink" href$="[[_computeDiffURL(change, patchRange.patchNum, patchRange.basePatchNum, file.__path)]]">
<a class="pathLink" href$="[[_computeDiffURL(change, patchRange.patchNum, patchRange.basePatchNum, file.__path, editMode)]]">
<span title$="[[computeDisplayPath(file.__path)]]"
class="fullFileName">
[[computeDisplayPath(file.__path)]]

9
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
View File

@@ -442,6 +442,7 @@
while (!row.classList.contains('row') && row.parentElement) {
row = row.parentElement;
}
const path = row.dataset.path;
// Handle checkbox mark as reviewed.
if (e.target.classList.contains('markReviewed')) {
@@ -667,7 +668,13 @@
return status || 'M';
},
_computeDiffURL(change, patchNum, basePatchNum, path) {
_computeDiffURL(change, patchNum, basePatchNum, path, editMode) {
// TODO(kaspern): Fix editing for commit messages and merge lists.
if (editMode && path !== this.COMMIT_MESSAGE_PATH &&
path !== this.MERGE_LIST_PATH) {
return Gerrit.Nav.getEditUrlForDiff(change, path, patchNum,
basePatchNum);
}
return Gerrit.Nav.getUrlForDiff(change, path, patchNum, basePatchNum);
},

1
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
View File

@@ -986,6 +986,7 @@ limitations under the License.
element.change = {_number: 123};
element.patchRange = {patchNum: undefined, basePatchNum: 'PARENT'};
element._files = [{__path: 'foo/bar.cpp'}];
element.editMode = false;
flush(() => {
assert.isFalse(urlStub.called);
element.set('patchRange.patchNum', 4);

3
polygerrit-ui/app/elements/edit/gr-edit-constants.html
View File

@@ -19,8 +19,9 @@ limitations under the License.
const GrEditConstants = window.GrEditConstants || {};
// Order corresponds to order in the UI.
GrEditConstants.Actions = {
EDIT: {label: 'Edit', id: 'edit'},
OPEN: {label: 'Open', id: 'open'},
DELETE: {label: 'Delete', id: 'delete'},
RENAME: {label: 'Rename', id: 'rename'},
RESTORE: {label: 'Restore', id: 'restore'},

10
polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.html
View File

@@ -79,13 +79,15 @@ limitations under the License.
</template>
<gr-overlay id="overlay" with-backdrop>
<gr-confirm-dialog
id="editDialog"
id="openDialog"
class="invisible dialog"
disabled$="[[!_isValidPath(_path)]]"
confirm-label="Edit"
on-confirm="_handleEditConfirm"
confirm-label="Open"
on-confirm="_handleOpenConfirm"
on-cancel="_handleDialogCancel">
<div class="header" slot="header">Edit a file</div>
<div class="header" slot="header">
Open an existing or new file
</div>
<div class="main" slot="main">
<gr-autocomplete
placeholder="Enter an existing or new full file path."

10
polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.js
View File

@@ -59,8 +59,8 @@
e.preventDefault();
const action = Polymer.dom(e).localTarget.id;
switch (action) {
case GrEditConstants.Actions.EDIT.id:
this.openEditDialog();
case GrEditConstants.Actions.OPEN.id:
this.openOpenDialog();
return;
case GrEditConstants.Actions.DELETE.id:
this.openDeleteDialog();
@@ -74,9 +74,9 @@
}
},
openEditDialog(opt_path) {
openOpenDialog(opt_path) {
if (opt_path) { this._path = opt_path; }
return this._showDialog(this.$.editDialog);
return this._showDialog(this.$.openDialog);
},
openDeleteDialog(opt_path) {
@@ -148,7 +148,7 @@
this._closeDialog(this._getDialogFromEvent(e));
},
_handleEditConfirm(e) {
_handleOpenConfirm(e) {
const url = Gerrit.Nav.getEditUrlForDiff(this.change, this._path,
this.patchNum);
Gerrit.Nav.navigateToRelativeUrl(url);

34
polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.html
View File

@@ -75,17 +75,17 @@ suite('gr-edit-controls tests', () => {
assert.isTrue(element._isValidPath('test.js'));
});
test('edit', () => {
MockInteractions.tap(element.$$('#edit'));
test('open', () => {
MockInteractions.tap(element.$$('#open'));
element.patchNum = 1;
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.editDialog.disabled);
assert.isTrue(element.$.openDialog.disabled);
assert.isFalse(queryStub.called);
element.$.editDialog.querySelector('gr-autocomplete').text =
element.$.openDialog.querySelector('gr-autocomplete').text =
'src/test.cpp';
assert.isTrue(queryStub.called);
assert.isFalse(element.$.editDialog.disabled);
MockInteractions.tap(element.$.editDialog.$$('gr-button[primary]'));
assert.isFalse(element.$.openDialog.disabled);
MockInteractions.tap(element.$.openDialog.$$('gr-button[primary]'));
for (const stub of navStubs) { assert.isTrue(stub.called); }
assert.deepEqual(Gerrit.Nav.getEditUrlForDiff.lastCall.args,
[element.change, 'src/test.cpp', element.patchNum]);
@@ -94,13 +94,13 @@ suite('gr-edit-controls tests', () => {
});
test('cancel', () => {
MockInteractions.tap(element.$$('#edit'));
MockInteractions.tap(element.$$('#open'));
return showDialogSpy.lastCall.returnValue.then(() => {
assert.isTrue(element.$.editDialog.disabled);
element.$.editDialog.querySelector('gr-autocomplete').text =
assert.isTrue(element.$.openDialog.disabled);
element.$.openDialog.querySelector('gr-autocomplete').text =
'src/test.cpp';
assert.isFalse(element.$.editDialog.disabled);
MockInteractions.tap(element.$.editDialog.$$('gr-button'));
assert.isFalse(element.$.openDialog.disabled);
MockInteractions.tap(element.$.openDialog.$$('gr-button'));
for (const stub of navStubs) { assert.isFalse(stub.called); }
assert.isTrue(closeDialogSpy.called);
assert.equal(element._path, 'src/test.cpp');
@@ -319,10 +319,10 @@ suite('gr-edit-controls tests', () => {
});
});
test('openEditDialog', () => {
return element.openEditDialog('test/path.cpp').then(() => {
assert.isFalse(element.$.editDialog.hasAttribute('hidden'));
assert.equal(element.$.editDialog.querySelector('gr-autocomplete').text,
test('openOpenDialog', () => {
return element.openOpenDialog('test/path.cpp').then(() => {
assert.isFalse(element.$.openDialog.hasAttribute('hidden'));
assert.equal(element.$.openDialog.querySelector('gr-autocomplete').text,
'test/path.cpp');
});
});
@@ -331,9 +331,9 @@ suite('gr-edit-controls tests', () => {
const spy = sandbox.spy(element, '_getDialogFromEvent');
element.addEventListener('tap', element._getDialogFromEvent);
MockInteractions.tap(element.$.editDialog);
MockInteractions.tap(element.$.openDialog);
flushAsynchronousOperations();
assert.equal(spy.lastCall.returnValue.id, 'editDialog');
assert.equal(spy.lastCall.returnValue.id, 'openDialog');
MockInteractions.tap(element.$.deleteDialog);
flushAsynchronousOperations();

15
polygerrit-ui/app/elements/edit/gr-edit-file-controls/gr-edit-file-controls.html
View File

@@ -30,11 +30,7 @@ limitations under the License.
display: flex;
justify-content: flex-end;
}
#edit {
text-decoration: none;
}
#edit,
#more {
#actions {
margin-right: 1em;
}
gr-button,
@@ -53,18 +49,13 @@ limitations under the License.
}
}
</style>
<gr-button
id="edit"
link
on-tap="_handleEditTap">Edit</gr-button>
<!-- TODO(kaspern): implement more menu. -->
<gr-dropdown
id="more"
id="actions"
items="[[_fileActions]]"
down-arrow
vertical-offset="20"
on-tap-item="_handleActionTap"
link>More</gr-dropdown>
link>Actions</gr-dropdown>
</template>
<script src="gr-edit-file-controls.js"></script>
</dom-module>

10
polygerrit-ui/app/elements/edit/gr-edit-file-controls/gr-edit-file-controls.js
View File

@@ -25,11 +25,9 @@
properties: {
filePath: String,
// Edit action not needed in the overflow.
_allFileActions: {
type: Array,
value: () => Object.values(GrEditConstants.Actions)
.filter(action => action !== GrEditConstants.Actions.EDIT),
value: () => Object.values(GrEditConstants.Actions),
},
_fileActions: {
type: Array,
@@ -37,12 +35,6 @@
},
},
_handleEditTap(e) {
e.preventDefault();
e.stopPropagation();
this._dispatchFileAction(GrEditConstants.Actions.EDIT.id, this.filePath);
},
_handleActionTap(e) {
e.preventDefault();
e.stopPropagation();

31
polygerrit-ui/app/elements/edit/gr-edit-file-controls/gr-edit-file-controls_test.html
View File

@@ -47,55 +47,56 @@ suite('gr-edit-file-controls tests', () => {
teardown(() => { sandbox.restore(); });
test('edit tap emits event', () => {
test('open tap emits event', () => {
const actions = element.$.actions;
element.filePath = 'foo';
actions._open();
flushAsynchronousOperations();
MockInteractions.tap(element.$.edit);
MockInteractions.tap(actions.$$('li [data-id="open"]'));
assert.isTrue(fileActionHandler.called);
assert.deepEqual(fileActionHandler.lastCall.args[0].detail,
{action: GrEditConstants.Actions.EDIT.id, path: 'foo'});
{action: GrEditConstants.Actions.OPEN.id, path: 'foo'});
});
test('delete tap emits event', () => {
const more = element.$.more;
const actions = element.$.actions;
element.filePath = 'foo';
more._open();
actions._open();
flushAsynchronousOperations();
MockInteractions.tap(more.$$('li [data-id="delete"]'));
MockInteractions.tap(actions.$$('li [data-id="delete"]'));
assert.isTrue(fileActionHandler.called);
assert.deepEqual(fileActionHandler.lastCall.args[0].detail,
{action: GrEditConstants.Actions.DELETE.id, path: 'foo'});
});
test('restore tap emits event', () => {
const more = element.$.more;
const actions = element.$.actions;
element.filePath = 'foo';
more._open();
actions._open();
flushAsynchronousOperations();
MockInteractions.tap(more.$$('li [data-id="restore"]'));
MockInteractions.tap(actions.$$('li [data-id="restore"]'));
assert.isTrue(fileActionHandler.called);
assert.deepEqual(fileActionHandler.lastCall.args[0].detail,
{action: GrEditConstants.Actions.RESTORE.id, path: 'foo'});
});
test('rename tap emits event', () => {
const more = element.$.more;
const actions = element.$.actions;
element.filePath = 'foo';
more._open();
actions._open();
flushAsynchronousOperations();
MockInteractions.tap(more.$$('li [data-id="rename"]'));
MockInteractions.tap(actions.$$('li [data-id="rename"]'));
assert.isTrue(fileActionHandler.called);
assert.deepEqual(fileActionHandler.lastCall.args[0].detail,
{action: GrEditConstants.Actions.RENAME.id, path: 'foo'});
});
test('computed properties', () => {
assert.equal(element._allFileActions.length, 3);
assert.notOk(element._allFileActions
.find(action => action.id === GrEditConstants.Actions.EDIT.id));
assert.equal(element._allFileActions.length, 4);
});
});
</script>