From 65e95674cdd33de67ac416764c3a9680829cf624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 3 Apr 2017 17:51:25 +0200 Subject: [PATCH] Factor out sorting in the doc table (#10997) Backports PR #10924 --- .../kibana/public/dashboard/panel/panel.html | 4 +- .../kibana/public/dashboard/panel/panel.js | 5 +- .../public/discover/controllers/discover.js | 4 + .../kibana/public/discover/index.html | 5 +- .../doc_table/__tests__/lib/rows_headers.js | 109 +++++++++++------- .../doc_table/components/table_header.html | 4 +- .../doc_table/components/table_header.js | 47 ++++---- src/ui/public/doc_table/doc_table.html | 15 ++- src/ui/public/doc_table/doc_table.js | 1 + 9 files changed, 120 insertions(+), 74 deletions(-) diff --git a/src/core_plugins/kibana/public/dashboard/panel/panel.html b/src/core_plugins/kibana/public/dashboard/panel/panel.html index 9f7368cb75a39..6d5cbef6c4f54 100644 --- a/src/core_plugins/kibana/public/dashboard/panel/panel.html +++ b/src/core_plugins/kibana/public/dashboard/panel/panel.html @@ -65,6 +65,8 @@ data-description="{{savedObj.description}}" render-counter class="panel-content" - filter="filter"> + filter="filter" + on-change-sort-order="setSortOrder" + > diff --git a/src/core_plugins/kibana/public/dashboard/panel/panel.js b/src/core_plugins/kibana/public/dashboard/panel/panel.js index 6f3875d8d6ffb..29408bf4ceb88 100644 --- a/src/core_plugins/kibana/public/dashboard/panel/panel.js +++ b/src/core_plugins/kibana/public/dashboard/panel/panel.js @@ -114,9 +114,10 @@ uiModules $scope.saveState(); }); - $scope.$watchCollection('panel.sort', function () { + $scope.setSortOrder = function setSortOrder(columnName, direction) { + $scope.panel.sort = [columnName, direction]; $scope.saveState(); - }); + }; } $scope.filter = function (field, value, operator) { diff --git a/src/core_plugins/kibana/public/discover/controllers/discover.js b/src/core_plugins/kibana/public/discover/controllers/discover.js index 6f469ee28eba4..ab1eb8a7f2064 100644 --- a/src/core_plugins/kibana/public/discover/controllers/discover.js +++ b/src/core_plugins/kibana/public/discover/controllers/discover.js @@ -475,6 +475,10 @@ function discoverController($scope, config, courier, $route, $window, Notifier, .set('filter', queryFilter.getFilters()); }); + $scope.setSortOrder = function setSortOrder(columnName, direction) { + $scope.state.sort = [columnName, direction]; + }; + // TODO: On array fields, negating does not negate the combination, rather all terms $scope.filterQuery = function (field, values, operation) { $scope.indexPattern.popularizeField(field, 1); diff --git a/src/core_plugins/kibana/public/discover/index.html b/src/core_plugins/kibana/public/discover/index.html index b4c647e9dddb4..bc183bd2c8bf7 100644 --- a/src/core_plugins/kibana/public/discover/index.html +++ b/src/core_plugins/kibana/public/discover/index.html @@ -131,8 +131,9 @@

Searching

shared-item data-title="{{opts.savedSearch.lastSavedTitle}}" data-description="{{opts.savedSearch.description}}" - render-counter> - + render-counter + on-change-sort-order="setSortOrder" + >
diff --git a/src/ui/public/doc_table/__tests__/lib/rows_headers.js b/src/ui/public/doc_table/__tests__/lib/rows_headers.js index fdef1f635351c..4ff71aa1fcbdd 100644 --- a/src/ui/public/doc_table/__tests__/lib/rows_headers.js +++ b/src/ui/public/doc_table/__tests__/lib/rows_headers.js @@ -8,6 +8,10 @@ import $ from 'jquery'; import 'plugins/kibana/discover/index'; import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern'; + +const SORTABLE_FIELDS = ['bytes', '@timestamp']; +const UNSORTABLE_FIELDS = ['request_body']; + describe('Doc Table', function () { let $parentScope; @@ -89,16 +93,24 @@ describe('Doc Table', function () { describe('kbnTableHeader', function () { - const $elem = angular.element( - '' - ); + const $elem = angular.element(` + + `); beforeEach(function () { init($elem, { columns: [], - sorting: [], + sortOrder: [], + onChangeSortOrder: sinon.stub(), }); }); + afterEach(function () { destroy(); }); @@ -107,57 +119,76 @@ describe('Doc Table', function () { columnTests('th', $elem); }); + describe('cycleSortOrder function', function () { + it('should exist', function () { + expect($scope.cycleSortOrder).to.be.a(Function); + }); - describe('sorting', function () { - it('should have a sort function that sets the elements of the sort array', function (done) { - expect($scope.sort).to.be.a(Function); - done(); + it('should call onChangeSortOrder with ascending order for a sortable field without sort order', function () { + $scope.sortOrder = []; + $scope.cycleSortOrder(SORTABLE_FIELDS[0]); + expect($scope.onChangeSortOrder.callCount).to.be(1); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'asc']); }); - it('should have a headClasser function that determines the css classes of the sort icons', function (done) { - expect($scope.headerClass).to.be.a(Function); - done(); + it('should call onChangeSortOrder with ascending order for a sortable field already sorted by in descending order', function () { + $scope.sortOrder = [SORTABLE_FIELDS[0], 'desc']; + $scope.cycleSortOrder(SORTABLE_FIELDS[0]); + expect($scope.onChangeSortOrder.callCount).to.be(1); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'asc']); }); - it('should sort asc by default, then by desc if already sorting', function (done) { - const fields = ['bytes', '@timestamp']; + it('should call onChangeSortOrder with ascending order for a sortable field when already sorted by an different field', function () { + $scope.sortOrder = [SORTABLE_FIELDS[1], 'asc']; + $scope.cycleSortOrder(SORTABLE_FIELDS[0]); + expect($scope.onChangeSortOrder.callCount).to.be(1); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'asc']); + }); - // Should not be sorted at first - expect($scope.sorting).to.eql(undefined); - expect($scope.headerClass(fields[0])).to.contain('fa-sort-up'); + it('should call onChangeSortOrder with descending order for a sortable field already sorted by in ascending order', function () { + $scope.sortOrder = [SORTABLE_FIELDS[0], 'asc']; + $scope.cycleSortOrder(SORTABLE_FIELDS[0]); + expect($scope.onChangeSortOrder.callCount).to.be(1); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'desc']); + }); - $scope.sort(fields[0]); - expect($scope.sorting).to.eql([fields[0], 'asc']); - expect($scope.headerClass(fields[0])).to.contain('fa-sort-up'); + it('should not call onChangeSortOrder for an unsortable field', function () { + $scope.sortOrder = []; + $scope.cycleSortOrder(UNSORTABLE_FIELDS[0]); + expect($scope.onChangeSortOrder.callCount).to.be(0); + }); - $scope.sort(fields[0]); - expect($scope.sorting).to.eql([fields[0], 'desc']); - expect($scope.headerClass(fields[0])).to.contain('fa-sort-down'); + it('should not try to call onChangeSortOrder when it is not defined', function () { + $scope.onChangeSortOrder = undefined; + expect(() => $scope.cycleSortOrder(SORTABLE_FIELDS[0])).to.not.throwException(); + }); + }); - $scope.sort(fields[0]); - expect($scope.sorting).to.eql([fields[0], 'asc']); - expect($scope.headerClass(fields[0])).to.contain('fa-sort-up'); + describe('headerClass function', function () { + it('should exist', function () { + expect($scope.headerClass).to.be.a(Function); + }); - $scope.sort(fields[1]); - expect($scope.sorting).to.eql([fields[1], 'asc']); - expect($scope.headerClass(fields[1])).to.contain('fa-sort-up'); + it('should return list including table-header-sortchange for a sortable field not currently sorted by', function () { + expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('table-header-sortchange'); + }); - // Should show the default sort for any other fields[0] - expect($scope.headerClass(fields[0])).to.contain('fa-sort-up'); + it('should return undefined for an unsortable field', function () { + expect($scope.headerClass(UNSORTABLE_FIELDS[0])).to.be(undefined); + }); - done(); + it('should return list including fa-sort-up for a sortable field not currently sorted by', function () { + expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-up'); }); - it('should NOT sort unindexed fields', function (done) { - $scope.sort('request_body'); - expect($scope.sorting).to.be(undefined); - done(); + it('should return list including fa-sort-up for a sortable field currently sorted by in ascending order', function () { + $scope.sortOrder = [SORTABLE_FIELDS[0], 'asc']; + expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-up'); }); - it('should NOT sort geo_point fields', function (done) { - $scope.sort('point'); - expect($scope.sorting).to.be(undefined); - done(); + it('should return list including fa-sort-down for a sortable field currently sorted by in descending order', function () { + $scope.sortOrder = [SORTABLE_FIELDS[0], 'desc']; + expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-down'); }); }); diff --git a/src/ui/public/doc_table/components/table_header.html b/src/ui/public/doc_table/components/table_header.html index 59c49bcd7d3e3..d3e71d9699751 100644 --- a/src/ui/public/doc_table/components/table_header.html +++ b/src/ui/public/doc_table/components/table_header.html @@ -1,11 +1,11 @@ - Time + Time - {{name | shortDots}} + {{name | shortDots}} diff --git a/src/ui/public/doc_table/components/table_header.js b/src/ui/public/doc_table/components/table_header.js index 68013a262d5b4..009c647f5b822 100644 --- a/src/ui/public/doc_table/components/table_header.js +++ b/src/ui/public/doc_table/components/table_header.js @@ -10,20 +10,22 @@ module.directive('kbnTableHeader', function (shortDotsFilter) { restrict: 'A', scope: { columns: '=', - sorting: '=', + sortOrder: '=', indexPattern: '=', + onChangeSortOrder: '=?', }, template: headerHtml, controller: function ($scope) { - - const sortableField = function (field) { - if (!$scope.indexPattern) return; - const sortable = _.get($scope.indexPattern.fields.byName[field], 'sortable'); - return sortable; + const isSortableColumn = function isSortableColumn(columnName) { + return ( + !!$scope.indexPattern + && _.isFunction($scope.onChangeSortOrder) + && _.get($scope, ['indexPattern', 'fields', 'byName', columnName, 'sortable'], false) + ); }; $scope.tooltip = function (column) { - if (!sortableField(column)) return ''; + if (!isSortableColumn(column)) return ''; return 'Sort by ' + shortDotsFilter(column); }; @@ -32,13 +34,13 @@ module.directive('kbnTableHeader', function (shortDotsFilter) { }; $scope.headerClass = function (column) { - if (!sortableField(column)) return; + if (!isSortableColumn(column)) return; - const sorting = $scope.sorting; + const sortOrder = $scope.sortOrder; const defaultClass = ['fa', 'fa-sort-up', 'table-header-sortchange']; - if (!sorting || column !== sorting[0]) return defaultClass; - return ['fa', sorting[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down']; + if (!sortOrder || column !== sortOrder[0]) return defaultClass; + return ['fa', sortOrder[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down']; }; $scope.moveLeft = function (column) { @@ -59,20 +61,19 @@ module.directive('kbnTableHeader', function (shortDotsFilter) { _.toggleInOut($scope.columns, fieldName); }; - $scope.sort = function (column) { - if (!column || !sortableField(column)) return; - - const sorting = $scope.sorting = $scope.sorting || []; - - let direction = sorting[1] || 'asc'; - if (sorting[0] !== column) { - direction = 'asc'; - } else { - direction = sorting[1] === 'asc' ? 'desc' : 'asc'; + $scope.cycleSortOrder = function cycleSortOrder(columnName) { + if (!isSortableColumn(columnName)) { + return; } - $scope.sorting[0] = column; - $scope.sorting[1] = direction; + const [currentColumnName, currentDirection = 'asc'] = $scope.sortOrder; + const newDirection = ( + (columnName === currentColumnName && currentDirection === 'asc') + ? 'desc' + : 'asc' + ); + + $scope.onChangeSortOrder(columnName, newDirection); }; } }; diff --git a/src/ui/public/doc_table/doc_table.html b/src/ui/public/doc_table/doc_table.html index 309a14fc449c2..35e972ba5630a 100644 --- a/src/ui/public/doc_table/doc_table.html +++ b/src/ui/public/doc_table/doc_table.html @@ -8,8 +8,9 @@ kbn-table-header columns="columns" index-pattern="indexPattern" - sorting="sorting"> - + sort-order="sorting" + on-change-sort-order="onChangeSortOrder" + > + class="discover-table-row" + on-change-sort-order="onChangeSortOrder" + > @@ -27,8 +30,9 @@ kbn-table-header columns="columns" index-pattern="indexPattern" - sorting="sorting"> - + sort-order="sorting" + on-change-sort-order="onChangeSortOrder" + > diff --git a/src/ui/public/doc_table/doc_table.js b/src/ui/public/doc_table/doc_table.js index a2223d1d3d73e..224d60f64c819 100644 --- a/src/ui/public/doc_table/doc_table.js +++ b/src/ui/public/doc_table/doc_table.js @@ -23,6 +23,7 @@ uiModules.get('kibana') searchSource: '=?', infiniteScroll: '=?', filter: '=?', + onChangeSortOrder: '=?', }, link: function ($scope) { const notify = new Notifier();