Merge "Modify change table editor so that all columns are in a single section"

2017-02-28 01:02:37 +00:00
parent 901a9e7db9 14aa6ab6fc
commit 94e093e7cb
10 changed files with 144 additions and 193 deletions
--- a/polygerrit-ui/app/behaviors/gr-change-table-behavior/gr-change-table-behavior.html
+++ b/polygerrit-ui/app/behaviors/gr-change-table-behavior/gr-change-table-behavior.html
@@ -19,22 +19,28 @@ limitations under the License.

  /** @polymerBehavior Gerrit.ChangeTableBehavior */
  var ChangeTableBehavior = {
-    CHANGE_TABLE_COLUMNS: [
-      'Subject',
-      'Status',
-      'Owner',
-      'Project',
-      'Branch',
-      'Updated',
-      'Size',
-    ],
+    properties: {
+      columnNames: {
+        type: Array,
+        value: [
+          'Subject',
+          'Status',
+          'Owner',
+          'Project',
+          'Branch',
+          'Updated',
+          'Size',
+        ],
+        readOnly: true,
+      }
+    },

    /**
     * Returns the complement to the given column array
     * @param {Array} columns
     */
    getComplementColumns: function(columns) {
-      return this.CHANGE_TABLE_COLUMNS.filter(function(column) {
+      return this.columnNames.filter(function(column) {
        return columns.indexOf(column) === -1;
      });
    },
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item_test.html
@@ -151,7 +151,7 @@ limitations under the License.

      flushAsynchronousOperations();

-      element.CHANGE_TABLE_COLUMNS.forEach(function(column) {
+      element.columnNames.forEach(function(column) {
        var elementClass = '.' + column.toLowerCase();
        assert.isFalse(element.$$(elementClass).hidden);
      });
@@ -170,7 +170,7 @@ limitations under the License.

      flushAsynchronousOperations();

-      element.CHANGE_TABLE_COLUMNS.forEach(function(column) {
+      element.columnNames.forEach(function(column) {
        var elementClass = '.' + column.toLowerCase();
        assert.isFalse(element.$$(elementClass).hidden);
      });
@@ -188,7 +188,7 @@ limitations under the License.

      flushAsynchronousOperations();

-      element.CHANGE_TABLE_COLUMNS.forEach(function(column) {
+      element.columnNames.forEach(function(column) {
        var elementClass = '.' + column.toLowerCase();
        if (column === 'Project') {
          assert.isTrue(element.$$(elementClass).hidden);
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js
@@ -111,18 +111,18 @@

    _loadPreferences: function() {
      return this._getLoggedIn().then(function(loggedIn) {
-        this.changeTableColumns = this.CHANGE_TABLE_COLUMNS;
+        this.changeTableColumns = this.columnNames;

        if (!loggedIn) {
          this.showNumber = false;
-          this.visibleChangeTableColumns = this.CHANGE_TABLE_COLUMNS;
+          this.visibleChangeTableColumns = this.columnNames;
          return;
        }
        return this._getPreferences().then(function(preferences) {
          this.showNumber = !!(preferences &&
              preferences.legacycid_in_change_table);
          this.visibleChangeTableColumns = preferences.change_table.length > 0 ?
-              preferences.change_table : this.CHANGE_TABLE_COLUMNS;
+              preferences.change_table : this.columnNames;
        }.bind(this));
      }.bind(this));
    },
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
@@ -266,7 +266,7 @@ limitations under the License.
      });

      test('all columns visible', function() {
-        element.CHANGE_TABLE_COLUMNS.forEach(function(column) {
+        element.columnNames.forEach(function(column) {
          var elementClass = '.' + element._lowerCase(column);
          assert.isFalse(element.$$(elementClass).hidden);
        });
--- 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
@@ -25,21 +25,19 @@ limitations under the License.
 <dom-module id="gr-change-table-editor">
  <template>
    <style>
+      table {
+        margin-top: 1em;
+      }
      th.nameHeader {
        width: 11em;
      }
-      tbody tr:first-of-type td .move-up-button,
-      tbody tr:last-of-type td .move-down-button {
-        display: none;
+      td.checkboxContainer {
+        border: 1px solid #fff;
+        cursor: pointer;
+        text-align: center;
      }
-      .newTitleInput {
-        width: 10em;
-      }
-      .newUrlInput {
-        width: 23em;
-      }
-      .addOptions {
-        margin-top: 1em;
+      td.checkboxContainer:hover {
+        border: 1px solid #ddd;
      }
    </style>
    <style include="gr-settings-styles"></style>
@@ -48,47 +46,24 @@ limitations under the License.
        <thead>
          <tr>
            <th class="nameHeader">Column</th>
+            <th>Visible</th>
          </tr>
        </thead>
        <tbody>
-          <template is="dom-repeat" items="[[changeTableItems]]">
+          <template is="dom-repeat" items="[[columnNames]]">
            <tr>
              <td>[[item]]</td>
-              <td>
-                <gr-button
-                    data-index="[[index]]"
-                    on-tap="_handleDeleteButton"
-                    class="remove-button">Delete</gr-button>
+              <td class="checkboxContainer"
+                  on-tap="_handleTargetTap">
+                <input
+                    type="checkbox"
+                    name="[[item]]"
+                    checked$="[[!isColumnHidden(item, displayedColumns)]]">
              </td>
            </tr>
          </template>
        </tbody>
      </table>
-
-      <template is="dom-if" if="[[changeTableNotDisplayed.length]]">
-        <table class="addOptions">
-          <thead>
-            <tr>
-              <th class="nameHeader">Hidden</th>
-            </tr>
-          </thead>
-          <tbody>
-            <template is="dom-repeat" items="[[changeTableNotDisplayed]]">
-              <tr>
-                <td>[[item]]</td>
-                <td>
-                  <gr-button
-                      on-tap="_handleAddButton"
-                      class="add-button"
-                      data-index="[[index]]">
-                    Add
-                  </gr-button>
-                </td>
-              </tr>
-            </template>
-          </tbody>
-        </table>
-      </template>
    </div>
  </template>
  <script src="gr-change-table-editor.js"></script>
--- 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
@@ -18,33 +18,44 @@
    is: 'gr-change-table-editor',

    properties: {
-      changeTableItems: Array,
-      changeTableNotDisplayed: Array,
+      displayedColumns: {
+        type: Array,
+        notify: true,
+      },
    },

    behaviors: [
      Gerrit.ChangeTableBehavior,
    ],

-    _handleDeleteButton: function(e) {
-      var index = e.target.dataIndex;
-      this.splice('changeTableItems', index, 1);
-
-      // Use the change table behavior to make sure ordering of unused
-      // columns ends up in the correct order. If the removed item is appended
-      // to the end, when it is saved, the unused column order may shift around.
-      this.set('changeTableNotDisplayed',
-          this.getComplementColumns(this.changeTableItems));
-
+    _getButtonText: function(isShown) {
+      return isShown ? 'Hide' : 'Show';
    },

-    _handleAddButton: function(e) {
-      var index = e.target.dataIndex;
-      var newColumn = this.changeTableNotDisplayed[index];
-      this.splice('changeTableNotDisplayed', index, 1);
+    _updateDisplayedColumns: function(displayedColumns, name, checked) {
+      if (!checked) {
+        return displayedColumns.filter(function(column) {
+          return name.toLowerCase() !== column.toLowerCase();
+        });
+      } else {
+        return displayedColumns.concat([name]);
+      }
+    },

-      this.splice('changeTableItems', this.getComplementColumns(
-          this.changeTableNotDisplayed).indexOf(newColumn), 0, newColumn);
+    /**
+     * Handles tap on either the checkbox itself or the surrounding table cell.
+     */
+    _handleTargetTap: function(e) {
+      var checkbox = Polymer.dom(e.target).querySelector('input');
+      if (checkbox) {
+        checkbox.click();
+      } else {
+        // The target is the checkbox itself.
+        checkbox = Polymer.dom(event).rootTarget;
+      }
+      this.set('displayedColumns',
+          this._updateDisplayedColumns(
+              this.displayedColumns, checkbox.name, checkbox.checked));
    },
  });
 })();
--- 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
@@ -31,22 +31,15 @@ limitations under the License.
 </test-fixture>

 <script>
-  suite('gr-settings-view tests', function() {
+  suite('gr-change-table-editor tests', function() {
    var element;
    var columns;
-
-    // Click the up/down button (according to direction) for the index'th row.
-    // The index of the first row is 0, corresponding to the array.
-    function move(element, index, direction) {
-      var selector =
-          'tr:nth-child(' + (index + 1) + ') .move-' + direction + '-button';
-      var button = element.$$('tbody').querySelector(selector);
-      MockInteractions.tap(button);
-      flushAsynchronousOperations();
-    }
+    var sandbox;

    setup(function() {
      element = fixture('basic');
+      sandbox = sinon.sandbox.create();
+
      columns = [
        'Subject',
        'Status',
@@ -56,138 +49,104 @@ limitations under the License.
        'Updated',
      ];

-      columnsNotDisplayed = ['Size'];
-
-      element.set('changeTableItems', columns);
-      element.set('changeTableNotDisplayed', columnsNotDisplayed);
+      element.set('displayedColumns', columns);
      flushAsynchronousOperations();
    });

+    teardown(function() {
+      sandbox.restore();
+    });
+
    test('renders', function() {
      var rows = element.$$('tbody').querySelectorAll('tr');
      var tds;

-      assert.equal(rows.length, columns.length);
+      assert.equal(rows.length, element.columnNames.length);
      for (var i = 0; i < columns.length; i++) {
        tds = rows[i].querySelectorAll('td');
        assert.equal(tds[0].textContent, columns[i]);
      }
    });

-    test('add hidden item', function() {
-      var originalNumberColumns = element.changeTableItems.length;
-      var originalNumberHiddenColumns = element.changeTableNotDisplayed.length;
+    test('hide item', function() {
+      var checkbox = element.$$('table input');
+      var isChecked = checkbox.checked;
+      var displayedLength = element.displayedColumns.length;
+      assert.isTrue(isChecked);

-      var addBtn = element.$$('.addOptions gr-button');
-      var columnName = element.$$('.addOptions tr td').innerHTML;
-
-      assert.equal(element.$$('.addOptions').style.display, '');
-
-      MockInteractions.tap(addBtn);
+      MockInteractions.tap(checkbox);
      flushAsynchronousOperations();

-      assert.equal(element.changeTableItems.length, originalNumberColumns + 1);
-      assert.equal(element.changeTableNotDisplayed.length,
-          originalNumberHiddenColumns - 1);
-
-      assert.equal(
-          element.changeTableItems[element.changeTableItems.length - 1],
-          columnName);
-
-      assert.equal(element.$$('.addOptions').style.display, 'none');
+      assert.equal(element.displayedColumns.length,
+          displayedLength - 1);
    });

-    test('remove item', function() {
-      var columns = element.changeTableItems.length;
-      assert.deepEqual(element.changeTableItems, [
-        'Subject',
+    test('show item', function() {
+      element.set('displayedColumns', [
        'Status',
        'Owner',
        'Project',
        'Branch',
        'Updated',
      ]);
+      flushAsynchronousOperations();
+      var checkbox = element.$$('table input');
+      var isChecked = checkbox.checked;
+      var displayedLength = element.displayedColumns.length;
+      assert.isFalse(isChecked);
+      assert.equal(element.$$('table').style.display, '');

-      // Tap the delete button for the second item.
-      MockInteractions.tap(
-          element.$$('tbody').querySelector('tr:nth-child(2) .remove-button'));
+      MockInteractions.tap(checkbox);
+      flushAsynchronousOperations();

-      assert.deepEqual(element.changeTableItems, [
-        'Subject',
-        'Owner',
-        'Project',
-        'Branch',
-        'Updated',
-      ]);
-
-      assert.deepEqual(element.changeTableNotDisplayed, [
-        'Status',
-        'Size',
-      ]);
-
-      // Delete remaining items.
-      for (var i = 0; i < columns - 1; i++) {
-        MockInteractions.tap(
-            element.$$('tbody').querySelector('tr:first-child .remove-button'));
-      }
-      assert.deepEqual(element.changeTableItems, []);
-      assert.deepEqual(element.changeTableNotDisplayed, [
-        'Subject',
-        'Status',
-        'Owner',
-        'Project',
-        'Branch',
-        'Updated',
-        'Size',
-      ]);
+      assert.equal(element.displayedColumns.length,
+          displayedLength + 1);
    });

-    test('add item', function() {
+    test('_handleTargetTap', function() {
+      var checkbox = element.$$('table input');
+      var originalDisplayedColumns = element.displayedColumns;
+      var td = element.$$('table .checkboxContainer');
+      var displayedColumnStub =
+          sandbox.stub(element, '_updateDisplayedColumns');

-      element.set('changeTableItems', [
-        'Status',
-        'Owner',
-        'Project',
-        'Branch',
-        'Updated',
-      ]);
+      MockInteractions.tap(checkbox);
+      assert.isTrue(displayedColumnStub.lastCall.calledWithExactly(
+          originalDisplayedColumns,
+          checkbox.name,
+          checkbox.checked));

-      element.set('changeTableNotDisplayed', ['Subject', 'Size']);
+      originalDisplayedColumns = element.displayedColumns;
+      MockInteractions.tap(td);
+      assert.isTrue(displayedColumnStub.lastCall.calledWithExactly(
+          originalDisplayedColumns,
+          checkbox.name,
+          checkbox.checked));
+    });

-      var columns = element.changeTableItems.length;
-      flushAsynchronousOperations();
-
-      // Tap the add button for the second item.
-      MockInteractions.tap(
-          element.$$('.addOptions').querySelector(
-              'tr:nth-child(2) .add-button'));
-
-      flushAsynchronousOperations();
-      assert.deepEqual(element.changeTableItems, [
-        'Status',
-        'Owner',
-        'Project',
-        'Branch',
-        'Updated',
-        'Size',
-      ]);
-      assert.deepEqual(element.changeTableNotDisplayed, ['Subject']);
-
-      // Add remaining item.
-      MockInteractions.tap(
-            element.$$('.addOptions').querySelector('.add-button'));
-      flushAsynchronousOperations();
-
-      assert.deepEqual(element.changeTableNotDisplayed, []);
-      assert.deepEqual(element.changeTableItems, [
-        'Subject',
-        'Status',
-        'Owner',
-        'Project',
-        'Branch',
-        'Updated',
-        'Size'
-      ]);
+    test('_updateDisplayedColumns', function() {
+      var name = 'Subject';
+      var checked = false;
+      assert.deepEqual(element._updateDisplayedColumns(columns, name, checked),
+          [
+            'Status',
+            'Owner',
+            'Project',
+            'Branch',
+            'Updated',
+          ]);
+      name = 'Size';
+      checked = true;
+      assert.deepEqual(element._updateDisplayedColumns(columns, name, checked),
+          [
+            'Subject',
+            'Status',
+            'Owner',
+            'Project',
+            'Branch',
+            'Updated',
+            'Size',
+          ]);
    });
  });
 </script>
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
@@ -314,8 +314,7 @@ limitations under the License.
        </h2>
        <fieldset id="changeTableColumns">
          <gr-change-table-editor
-              change-table-items="{{_localChangeTableColumns}}"
-              change-table-not-displayed="{{_changeTableColumnsNotDisplayed}}">
+              displayed-columns="{{_localChangeTableColumns}}">
          </gr-change-table-editor>
          <gr-button
              id="saveChangeTable"
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
@@ -117,7 +117,7 @@
      '_handlePrefsChanged(_localPrefs.*)',
      '_handleDiffPrefsChanged(_diffPrefs.*)',
      '_handleMenuChanged(_localMenu.splices)',
-      '_handleChangeTableChanged(_localChangeTableColumns.splices)',
+      '_handleChangeTableChanged(_localChangeTableColumns)',
    ],

    attached: function() {
@@ -220,7 +220,7 @@
      var columns = this.prefs.change_table;

      if (columns.length === 0) {
-        columns = this.CHANGE_TABLE_COLUMNS;
+        columns = this.columnNames;
        this._changeTableColumnsNotDisplayed = [];
      } else {
        this._changeTableColumnsNotDisplayed = this.getComplementColumns(
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -71,6 +71,7 @@ limitations under the License.
    'diff/gr-syntax-lib-loader/gr-syntax-lib-loader_test.html',
    'gr-app_test.html',
    'settings/gr-account-info/gr-account-info_test.html',
+    'settings/gr-change-table-editor/gr-change-table-editor_test.html',
    'settings/gr-email-editor/gr-email-editor_test.html',
    'settings/gr-group-list/gr-group-list_test.html',
    'settings/gr-http-password/gr-http-password_test.html',