From 91305bd62fbaf0b6083a0e902db88107927b8644 Mon Sep 17 00:00:00 2001 From: Matt Bargar Date: Fri, 2 Aug 2019 17:18:54 -0400 Subject: [PATCH] Allow sorting on multiple columns in Discover (#41918) This commit enables sorting on multiple columns in Discover and in saved search panels on dashboards. The UI is simple and should be familiar based on the way multi-sort works in other common applications like file explorers. Each sortable column has a sort icon indicating which way it is sorted (or unsorted, in the case of two arrows pointing both up and down). Sort priority is determined by which column was clicked most recently, with the most recent being the lowest priority. --- .../public/discover/controllers/discover.js | 8 +- .../doc_table/__tests__/lib/get_sort.js | 20 ++-- .../table_header/table_header.test.tsx | 4 +- .../components/table_header/table_header.tsx | 7 +- .../table_header/table_header_column.tsx | 98 +++++++++++++------ .../public/discover/doc_table/doc_table.html | 2 - .../public/discover/doc_table/lib/get_sort.js | 53 +++++----- .../discover/embeddable/search_embeddable.ts | 11 ++- .../styles/_legacy/components/_table.scss | 4 +- .../functional/apps/discover/_shared_links.js | 2 +- .../translations/translations/ja-JP.json | 1 - .../translations/translations/zh-CN.json | 1 - 12 files changed, 129 insertions(+), 82 deletions(-) diff --git a/src/legacy/core_plugins/kibana/public/discover/controllers/discover.js b/src/legacy/core_plugins/kibana/public/discover/controllers/discover.js index 4711fb1edb018..377fd72e9c771 100644 --- a/src/legacy/core_plugins/kibana/public/discover/controllers/discover.js +++ b/src/legacy/core_plugins/kibana/public/discover/controllers/discover.js @@ -554,8 +554,8 @@ function discoverController( $scope.$watchCollection('state.sort', function (sort) { if (!sort) return; - // get the current sort from {key: val} to ["key", "val"]; - const currentSort = _.pairs($scope.searchSource.getField('sort')).pop(); + // get the current sort from searchSource as array of arrays + const currentSort = getSort.array($scope.searchSource.getField('sort'), $scope.indexPattern); // if the searchSource doesn't know, tell it so if (!angular.equals(sort, currentSort)) $scope.fetch(); @@ -862,8 +862,8 @@ function discoverController( .setField('filter', queryFilter.getFilters()); }); - $scope.setSortOrder = function setSortOrder(columnName, direction) { - $scope.state.sort = [columnName, direction]; + $scope.setSortOrder = function setSortOrder(sortPair) { + $scope.state.sort = sortPair; }; // TODO: On array fields, negating does not negate the combination, rather all terms diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/get_sort.js b/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/get_sort.js index bec26d38d0fa7..b8b962b9f92d7 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/get_sort.js +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/get_sort.js @@ -23,7 +23,7 @@ import ngMock from 'ng_mock'; import { getSort } from '../../lib/get_sort'; import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern'; -const defaultSort = { time: 'desc' }; +const defaultSort = [{ time: 'desc' }]; let indexPattern; describe('docTable', function () { @@ -38,11 +38,11 @@ describe('docTable', function () { expect(getSort).to.be.a(Function); }); - it('should return an object if passed a 2 item array', function () { - expect(getSort(['bytes', 'desc'], indexPattern)).to.eql({ bytes: 'desc' }); + it('should return an array of objects if passed a 2 item array', function () { + expect(getSort(['bytes', 'desc'], indexPattern)).to.eql([{ bytes: 'desc' }]); delete indexPattern.timeFieldName; - expect(getSort(['bytes', 'desc'], indexPattern)).to.eql({ bytes: 'desc' }); + expect(getSort(['bytes', 'desc'], indexPattern)).to.eql([{ bytes: 'desc' }]); }); it('should sort by the default when passed an unsortable field', function () { @@ -50,7 +50,7 @@ describe('docTable', function () { expect(getSort(['lol_nope', 'asc'], indexPattern)).to.eql(defaultSort); delete indexPattern.timeFieldName; - expect(getSort(['non-sortable', 'asc'], indexPattern)).to.eql({ _score: 'desc' }); + expect(getSort(['non-sortable', 'asc'], indexPattern)).to.eql([{ _score: 'desc' }]); }); it('should sort in reverse chrono order otherwise on time based patterns', function () { @@ -62,9 +62,9 @@ describe('docTable', function () { it('should sort by score on non-time patterns', function () { delete indexPattern.timeFieldName; - expect(getSort([], indexPattern)).to.eql({ _score: 'desc' }); - expect(getSort(['foo'], indexPattern)).to.eql({ _score: 'desc' }); - expect(getSort({ foo: 'bar' }, indexPattern)).to.eql({ _score: 'desc' }); + expect(getSort([], indexPattern)).to.eql([{ _score: 'desc' }]); + expect(getSort(['foo'], indexPattern)).to.eql([{ _score: 'desc' }]); + expect(getSort({ foo: 'bar' }, indexPattern)).to.eql([{ _score: 'desc' }]); }); }); @@ -73,8 +73,8 @@ describe('docTable', function () { expect(getSort.array).to.be.a(Function); }); - it('should return an array for sortable fields', function () { - expect(getSort.array(['bytes', 'desc'], indexPattern)).to.eql([ 'bytes', 'desc' ]); + it('should return an array of arrays for sortable fields', function () { + expect(getSort.array(['bytes', 'desc'], indexPattern)).to.eql([[ 'bytes', 'desc' ]]); }); }); }); diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.test.tsx b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.test.tsx index 20fd8edaf6784..62b8ece1aff46 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.test.tsx +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.test.tsx @@ -59,7 +59,7 @@ function getMockProps(props = {}) { indexPattern: getMockIndexPattern(), hideTimeColumn: false, columns: ['first', 'middle', 'last'], - sortOrder: ['time', 'asc'] as SortOrder, + sortOrder: [['time', 'asc']] as SortOrder[], isShortDots: true, onRemoveColumn: jest.fn(), onChangeSortOrder: jest.fn(), @@ -89,7 +89,7 @@ describe('TableHeader with time column', () => { test('time column is sortable with button, cycling sort direction', () => { findTestSubject(wrapper, 'docTableHeaderFieldSort_time').simulate('click'); - expect(props.onChangeSortOrder).toHaveBeenCalledWith('time', 'desc'); + expect(props.onChangeSortOrder).toHaveBeenCalledWith([['time', 'desc']]); }); test('time column is not removeable, no button displayed', () => { diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.tsx b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.tsx index 62799bf80e96c..d70fe98dba381 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.tsx +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header.tsx @@ -28,10 +28,10 @@ interface Props { hideTimeColumn: boolean; indexPattern: IndexPatternEnhanced; isShortDots: boolean; - onChangeSortOrder?: (name: string, direction: 'asc' | 'desc') => void; + onChangeSortOrder?: (sortOrder: SortOrder[]) => void; onMoveColumn?: (name: string, index: number) => void; onRemoveColumn?: (name: string) => void; - sortOrder: SortOrder; + sortOrder: SortOrder[]; } export function TableHeader({ @@ -45,7 +45,6 @@ export function TableHeader({ sortOrder, }: Props) { const displayedColumns = getDisplayedColumns(columns, indexPattern, hideTimeColumn, isShortDots); - const [currColumnName, currDirection = 'asc'] = sortOrder; return ( @@ -55,7 +54,7 @@ export function TableHeader({ void; + onChangeSortOrder?: (sortOrder: SortOrder[]) => void; onMoveColumn?: (name: string, idx: number) => void; onRemoveColumn?: (name: string) => void; - sortDirection: 'asc' | 'desc' | ''; // asc|desc -> field is sorted in this direction, else '' + sortOrder: SortOrder[]; } +const sortDirectionToIcon = { + desc: 'fa fa-sort-down', + asc: 'fa fa-sort-up', + '': 'fa fa-sort', +}; + export function TableHeaderColumn({ colLeftIdx, colRightIdx, @@ -46,43 +53,78 @@ export function TableHeaderColumn({ onChangeSortOrder, onMoveColumn, onRemoveColumn, - sortDirection, + sortOrder, }: Props) { - const btnSortIcon = sortDirection === 'desc' ? 'fa fa-sort-down' : 'fa fa-sort-up'; + const [, sortDirection = ''] = sortOrder.find(sortPair => name === sortPair[0]) || []; + const currentSortWithoutColumn = sortOrder.filter(pair => pair[0] !== name); + const currentColumnSort = sortOrder.find(pair => pair[0] === name); + const currentColumnSortDirection = (currentColumnSort && currentColumnSort[1]) || ''; + + const btnSortIcon = sortDirectionToIcon[sortDirection]; const btnSortClassName = sortDirection !== '' ? btnSortIcon : `kbnDocTableHeader__sortChange ${btnSortIcon}`; + const handleChangeSortOrder = () => { + if (!onChangeSortOrder) return; + + // Cycle goes Unsorted -> Asc -> Desc -> Unsorted + if (currentColumnSort === undefined) { + onChangeSortOrder([...currentSortWithoutColumn, [name, 'asc']]); + } else if (currentColumnSortDirection === 'asc') { + onChangeSortOrder([...currentSortWithoutColumn, [name, 'desc']]); + } else if (currentColumnSortDirection === 'desc' && currentSortWithoutColumn.length === 0) { + // If we're at the end of the cycle and this is the only existing sort, we switch + // back to ascending sort instead of removing it. + onChangeSortOrder([[name, 'asc']]); + } else { + onChangeSortOrder(currentSortWithoutColumn); + } + }; + + const getSortButtonAriaLabel = () => { + const sortAscendingMessage = i18n.translate( + 'kbn.docTable.tableHeader.sortByColumnAscendingAriaLabel', + { + defaultMessage: 'Sort {columnName} ascending', + values: { columnName: name }, + } + ); + const sortDescendingMessage = i18n.translate( + 'kbn.docTable.tableHeader.sortByColumnDescendingAriaLabel', + { + defaultMessage: 'Sort {columnName} descending', + values: { columnName: name }, + } + ); + const stopSortingMessage = i18n.translate( + 'kbn.docTable.tableHeader.sortByColumnUnsortedAriaLabel', + { + defaultMessage: 'Stop sorting on {columnName}', + values: { columnName: name }, + } + ); + + if (currentColumnSort === undefined) { + return sortAscendingMessage; + } else if (sortDirection === 'asc') { + return sortDescendingMessage; + } else if (sortDirection === 'desc' && currentSortWithoutColumn.length === 0) { + return sortAscendingMessage; + } else { + return stopSortingMessage; + } + }; + // action buttons displayed on the right side of the column name const buttons = [ // Sort Button { active: isSortable && typeof onChangeSortOrder === 'function', - ariaLabel: - sortDirection === 'asc' - ? i18n.translate('kbn.docTable.tableHeader.sortByColumnDescendingAriaLabel', { - defaultMessage: 'Sort {columnName} descending', - values: { columnName: name }, - }) - : i18n.translate('kbn.docTable.tableHeader.sortByColumnAscendingAriaLabel', { - defaultMessage: 'Sort {columnName} ascending', - values: { columnName: name }, - }), + ariaLabel: getSortButtonAriaLabel(), className: btnSortClassName, - onClick: () => { - /** - * cycle sorting direction - * asc -> desc, desc -> asc, default: asc - */ - if (typeof onChangeSortOrder === 'function') { - const newDirection = sortDirection === 'asc' ? 'desc' : 'asc'; - onChangeSortOrder(name, newDirection); - } - }, + onClick: handleChangeSortOrder, testSubject: `docTableHeaderFieldSort_${name}`, - tooltip: i18n.translate('kbn.docTable.tableHeader.sortByColumnTooltip', { - defaultMessage: 'Sort by {columnName}', - values: { columnName: name }, - }), + tooltip: getSortButtonAriaLabel(), }, // Remove Button { diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.html b/src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.html index 075468b76090b..b73f204626b9c 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.html +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/doc_table.html @@ -46,7 +46,6 @@ filters="filters" class="kbnDocTable__row" on-add-column="onAddColumn" - on-change-sort-order="onChangeSortOrder" on-remove-column="onRemoveColumn" > @@ -98,7 +97,6 @@ ng-class="{'kbnDocTable__row--highlight': row['$$_isAnchor']}" data-test-subj="docTableRow{{ row['$$_isAnchor'] ? ' docTableAnchorRow' : ''}}" on-add-column="onAddColumn" - on-change-sort-order="onChangeSortOrder" on-remove-column="onRemoveColumn" > diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/lib/get_sort.js b/src/legacy/core_plugins/kibana/public/discover/doc_table/lib/get_sort.js index 82195823c6749..9aba887569dbf 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/lib/get_sort.js +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/lib/get_sort.js @@ -19,42 +19,49 @@ import _ from 'lodash'; +function isSortable(field, indexPattern) { + return (indexPattern.fields.byName[field] && indexPattern.fields.byName[field].sortable); +} + +function createSortObject(sortPair, indexPattern) { + + if (Array.isArray(sortPair) && sortPair.length === 2 && isSortable(sortPair[0], indexPattern)) { + const [ field, direction ] = sortPair; + return { [field]: direction }; + } + else { + return undefined; + } +} + /** * Take a sorting array and make it into an object - * @param {array} 2 item array [fieldToSort, directionToSort] + * @param {array} sort 2 item array [fieldToSort, directionToSort] * @param {object} indexPattern used for determining default sort * @returns {object} a sort object suitable for returning to elasticsearch */ export function getSort(sort, indexPattern, defaultSortOrder = 'desc') { - const sortObj = {}; - let field; - let direction; - function isSortable(field) { - return (indexPattern.fields.byName[field] && indexPattern.fields.byName[field].sortable); + let sortObjects; + if (Array.isArray(sort)) { + if (sort.length > 0 && !Array.isArray(sort[0])) { + sort = [sort]; + } + sortObjects = _.compact(sort.map((sortPair) => createSortObject(sortPair, indexPattern))); } - if (Array.isArray(sort) && sort.length === 2 && isSortable(sort[0])) { - // At some point we need to refactor the sorting logic, this array sucks. - field = sort[0]; - direction = sort[1]; - } else if (indexPattern.timeFieldName && isSortable(indexPattern.timeFieldName)) { - field = indexPattern.timeFieldName; - direction = defaultSortOrder; + if (!_.isEmpty(sortObjects)) { + return sortObjects; } - - if (field) { - sortObj[field] = direction; - } else { - sortObj._score = 'desc'; + else if (indexPattern.timeFieldName && isSortable(indexPattern.timeFieldName, indexPattern)) { + return [{ [indexPattern.timeFieldName]: defaultSortOrder }]; + } + else { + return [{ _score: 'desc' }]; } - - - - return sortObj; } getSort.array = function (sort, indexPattern, defaultSortOrder) { - return _(getSort(sort, indexPattern, defaultSortOrder)).pairs().pop(); + return getSort(sort, indexPattern, defaultSortOrder).map((sortPair) => _(sortPair).pairs().pop()); }; diff --git a/src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable.ts b/src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable.ts index 7dec893f68025..56f368f2a5161 100644 --- a/src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable.ts +++ b/src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable.ts @@ -45,11 +45,11 @@ import { ISearchEmbeddable, SearchInput, SearchOutput } from './types'; interface SearchScope extends ng.IScope { columns?: string[]; description?: string; - sort?: string[]; + sort?: string[] | string[][]; searchSource?: SearchSource; sharedItemTitle?: string; inspectorAdapters?: Adapters; - setSortOrder?: (column: string, columnDirection: string) => void; + setSortOrder?: (sortPair: [string, string]) => void; removeColumn?: (column: string) => void; addColumn?: (column: string) => void; moveColumn?: (column: string, index: number) => void; @@ -201,8 +201,8 @@ export class SearchEmbeddable extends Embeddable this.pushContainerStateParamsToScope(searchScope); - searchScope.setSortOrder = (columnName, direction) => { - searchScope.sort = [columnName, direction]; + searchScope.setSortOrder = sortPair => { + searchScope.sort = sortPair; this.updateInput({ sort: searchScope.sort }); }; @@ -263,6 +263,9 @@ export class SearchEmbeddable extends Embeddable // been overridden in a dashboard. searchScope.columns = this.input.columns || this.savedSearch.columns; searchScope.sort = this.input.sort || this.savedSearch.sort; + if (searchScope.sort.length && !Array.isArray(searchScope.sort[0])) { + searchScope.sort = [searchScope.sort]; + } searchScope.sharedItemTitle = this.panelTitle; if ( diff --git a/src/legacy/ui/public/styles/_legacy/components/_table.scss b/src/legacy/ui/public/styles/_legacy/components/_table.scss index 94f30fa1f3498..e7c1bda829f0e 100644 --- a/src/legacy/ui/public/styles/_legacy/components/_table.scss +++ b/src/legacy/ui/public/styles/_legacy/components/_table.scss @@ -33,14 +33,14 @@ table { button.fa-sort-down, i.fa-sort-asc, i.fa-sort-down { - color: $euiColorLightShade; + color: $euiColorPrimary; } button.fa-sort-desc, button.fa-sort-up, i.fa-sort-desc, i.fa-sort-up { - color: $euiColorLightShade; + color: $euiColorPrimary; } } } diff --git a/test/functional/apps/discover/_shared_links.js b/test/functional/apps/discover/_shared_links.js index 4b85593d1f02c..f0d34207a87a2 100644 --- a/test/functional/apps/discover/_shared_links.js +++ b/test/functional/apps/discover/_shared_links.js @@ -71,7 +71,7 @@ export default function ({ getService, getPageObjects }) { ':(from:\'2015-09-19T06:31:44.000Z\',to:\'2015-09' + '-23T18:31:44.000Z\'))&_a=(columns:!(_source),index:\'logstash-' + '*\',interval:auto,query:(language:kuery,query:\'\')' + - ',sort:!(\'@timestamp\',desc))'; + ',sort:!(!(\'@timestamp\',desc)))'; const actualUrl = await PageObjects.share.getSharedUrl(); // strip the timestamp out of each URL expect(actualUrl.replace(/_t=\d{13}/, '_t=TIMESTAMP')).to.be( diff --git a/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json index 0096654281044..796e2ce2850b0 100644 --- a/x-pack/plugins/translations/translations/ja-JP.json +++ b/x-pack/plugins/translations/translations/ja-JP.json @@ -1583,7 +1583,6 @@ "kbn.docTable.tableHeader.removeColumnButtonTooltip": "列を削除", "kbn.docTable.tableHeader.sortByColumnAscendingAriaLabel": "{columnName} を昇順に並べ替える", "kbn.docTable.tableHeader.sortByColumnDescendingAriaLabel": "{columnName} を降順に並べ替える", - "kbn.docTable.tableHeader.sortByColumnTooltip": "{columnName} で並べ替えます", "kbn.docTable.tableRow.detailHeading": "拡張ドキュメント", "kbn.docTable.tableRow.filterForValueButtonAriaLabel": "値でフィルタリング", "kbn.docTable.tableRow.filterForValueButtonTooltip": "値でフィルタリング", diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json index 0e04a11bbf833..bfa4eb51b6ee1 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -1584,7 +1584,6 @@ "kbn.docTable.tableHeader.removeColumnButtonTooltip": "删除列", "kbn.docTable.tableHeader.sortByColumnAscendingAriaLabel": "升序排序 {columnName}", "kbn.docTable.tableHeader.sortByColumnDescendingAriaLabel": "降序排序 {columnName}", - "kbn.docTable.tableHeader.sortByColumnTooltip": "按“{columnName}”排序", "kbn.docTable.tableRow.detailHeading": "已展开文档", "kbn.docTable.tableRow.filterForValueButtonAriaLabel": "筛留值", "kbn.docTable.tableRow.filterForValueButtonTooltip": "筛留值",