Prevent duplicates in change_table preference
The change table editor is used to modify what columns appear in change
lists with a series of checkboxes. These checkboxes have a click area
that is slightly larger than the checkbox itself in order to make them
easier to click on. This is accomplished by intercepting a click on the
container and relaying it to the checkbox it contains.
However, because one event handler was used for three different
purposes, every time a user clicked on the checkbox container this would
result in two calls to the handler. If the column was being added, this
would mean that it would appear in the preference list twice.
Duplicating preference names is avoided by splitting the event handlers
so that each handler does one thing only. Updates to the preference list
are made my querying the state of the checkboxes rather than making
incremental updates.
The cursor style of the checkbox is updated too.
Change-Id: Icb8c4b94209c62c7794112f36e89183776c73ea8
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html
index fa188d7..4f69513 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html
@@ -37,6 +37,9 @@
cursor: pointer;
text-align: center;
}
+ .checkboxContainer input {
+ cursor: pointer;
+ }
.checkboxContainer:hover {
outline: 1px solid var(--border-color);
}
@@ -52,12 +55,12 @@
<tbody>
<tr>
<td>Number</td>
- <td
- class="checkboxContainer"
- on-tap="_handleTargetTap">
+ <td class="checkboxContainer"
+ on-tap="_handleCheckboxContainerTap">
<input
type="checkbox"
name="number"
+ on-tap="_handleNumberCheckboxTap"
checked$="[[showNumber]]">
</td>
</tr>
@@ -65,10 +68,11 @@
<tr>
<td>[[item]]</td>
<td class="checkboxContainer"
- on-tap="_handleTargetTap">
+ on-tap="_handleCheckboxContainerTap">
<input
type="checkbox"
name="[[item]]"
+ on-tap="_handleTargetTap"
checked$="[[!isColumnHidden(item, displayedColumns)]]">
</td>
</tr>
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js
index 7b74096..7d109633 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js
@@ -35,40 +35,42 @@
Gerrit.ChangeTableBehavior,
],
- _getButtonText(isShown) {
- return isShown ? 'Hide' : 'Show';
- },
-
- _updateDisplayedColumns(displayedColumns, name, checked) {
- if (!checked) {
- return displayedColumns.filter(column => {
- return name.toLowerCase() !== column.toLowerCase();
- });
- } else {
- return displayedColumns.concat([name]);
- }
+ /**
+ * Get the list of enabled column names from whichever checkboxes are
+ * checked (excluding the number checkbox).
+ * @return {!Array<string>}
+ */
+ _getDisplayedColumns() {
+ return Polymer.dom(this.root)
+ .querySelectorAll('.checkboxContainer input:not([name=number])')
+ .filter(checkbox => checkbox.checked)
+ .map(checkbox => checkbox.name);
},
/**
- * Handles tap on either the checkbox itself or the surrounding table cell.
+ * Handle a tap on a checkbox container and relay the tap to the checkbox it
+ * contains.
+ */
+ _handleCheckboxContainerTap(e) {
+ const checkbox = Polymer.dom(e.target).querySelector('input');
+ if (!checkbox) { return; }
+ checkbox.click();
+ },
+
+ /**
+ * Handle a tap on the number checkbox and update the showNumber property
+ * accordingly.
+ */
+ _handleNumberCheckboxTap(e) {
+ this.showNumber = Polymer.dom(e).rootTarget.checked;
+ },
+
+ /**
+ * Handle a tap on a displayed column checkboxes (excluding number) and
+ * update the displayedColumns property accordingly.
*/
_handleTargetTap(e) {
- let checkbox = Polymer.dom(e.target).querySelector('input');
- if (checkbox) {
- checkbox.click();
- } else {
- // The target is the checkbox itself.
- checkbox = Polymer.dom(e).rootTarget;
- }
-
- if (checkbox.name === 'number') {
- this.showNumber = checkbox.checked;
- return;
- }
-
- this.set('displayedColumns',
- this._updateDisplayedColumns(
- this.displayedColumns, checkbox.name, checkbox.checked));
+ this.set('displayedColumns', this._getDisplayedColumns());
},
});
})();
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html
index 587cc3b..32fab9d 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html
@@ -53,6 +53,7 @@
];
element.set('displayedColumns', columns);
+ element.showNumber = false;
flushAsynchronousOperations();
});
@@ -108,66 +109,50 @@
displayedLength + 1);
});
- test('_handleTargetTap', () => {
- const checkbox = element.$$('table tr:nth-child(2) input');
- let originalDisplayedColumns = element.displayedColumns;
- const td = element.$$('table tr:nth-child(2) .checkboxContainer');
- const displayedColumnStub =
- sandbox.stub(element, '_updateDisplayedColumns');
-
- MockInteractions.tap(checkbox);
- assert.isTrue(displayedColumnStub.lastCall.calledWithExactly(
- originalDisplayedColumns,
- checkbox.name,
- checkbox.checked));
-
- originalDisplayedColumns = element.displayedColumns;
- MockInteractions.tap(td);
- assert.isTrue(displayedColumnStub.lastCall.calledWithExactly(
- originalDisplayedColumns,
- checkbox.name,
- checkbox.checked));
+ test('_getDisplayedColumns', () => {
+ assert.deepEqual(element._getDisplayedColumns(), columns);
+ MockInteractions.tap(
+ element.$$('.checkboxContainer input[name=Assignee]'));
+ assert.deepEqual(element._getDisplayedColumns(),
+ columns.filter(c => c !== 'Assignee'));
});
- test('_handleTargetTap on number', () => {
- element.showNumber = false;
- const checkbox = element.$$('table tr:nth-child(1) input');
- const displayedColumnStub =
- sandbox.stub(element, '_updateDisplayedColumns');
+ test('_handleCheckboxContainerTap relayes taps to checkboxes', () => {
+ sandbox.stub(element, '_handleNumberCheckboxTap');
+ sandbox.stub(element, '_handleTargetTap');
- MockInteractions.tap(checkbox);
- assert.isFalse(displayedColumnStub.called);
+ MockInteractions.tap(
+ element.$$('table tr:first-of-type .checkboxContainer'));
+ assert.isTrue(element._handleNumberCheckboxTap.calledOnce);
+ assert.isFalse(element._handleTargetTap.called);
+
+ MockInteractions.tap(
+ element.$$('table tr:last-of-type .checkboxContainer'));
+ assert.isTrue(element._handleNumberCheckboxTap.calledOnce);
+ assert.isTrue(element._handleTargetTap.calledOnce);
+ });
+
+ test('_handleNumberCheckboxTap', () => {
+ sandbox.spy(element, '_handleNumberCheckboxTap');
+
+ MockInteractions
+ .tap(element.$$('.checkboxContainer input[name=number]'));
+ assert.isTrue(element._handleNumberCheckboxTap.calledOnce);
assert.isTrue(element.showNumber);
- MockInteractions.tap(checkbox);
+ MockInteractions
+ .tap(element.$$('.checkboxContainer input[name=number]'));
+ assert.isTrue(element._handleNumberCheckboxTap.calledTwice);
assert.isFalse(element.showNumber);
});
- test('_updateDisplayedColumns', () => {
- let name = 'Subject';
- let checked = false;
- assert.deepEqual(element._updateDisplayedColumns(columns, name, checked),
- [
- 'Status',
- 'Owner',
- 'Assignee',
- 'Repo',
- 'Branch',
- 'Updated',
- ]);
- name = 'Size';
- checked = true;
- assert.deepEqual(element._updateDisplayedColumns(columns, name, checked),
- [
- 'Subject',
- 'Status',
- 'Owner',
- 'Assignee',
- 'Repo',
- 'Branch',
- 'Updated',
- 'Size',
- ]);
+ test('_handleTargetTap', () => {
+ sandbox.spy(element, '_handleTargetTap');
+ assert.include(element.displayedColumns, 'Assignee');
+ MockInteractions
+ .tap(element.$$('.checkboxContainer input[name=Assignee]'));
+ assert.isTrue(element._handleTargetTap.calledOnce);
+ assert.notInclude(element.displayedColumns, 'Assignee');
});
});
</script>
