From 140b0f45c33af0db7c7d3c2a69f1d67e4f053740 Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Wed, 24 Jul 2019 14:03:15 -0400 Subject: [PATCH 1/9] Allow sorting on multiple columns in Discover --- .../public/discover/controllers/discover.js | 8 +-- .../doc_table/components/table_header.js | 32 +++++++---- .../public/discover/doc_table/doc_table.html | 2 - .../public/discover/doc_table/lib/get_sort.js | 53 +++++++++++-------- .../discover/embeddable/search_embeddable.ts | 11 ++-- 5 files changed, 62 insertions(+), 44 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 94a62511bc2ad..4189dad577e27 100644 --- a/src/legacy/core_plugins/kibana/public/discover/controllers/discover.js +++ b/src/legacy/core_plugins/kibana/public/discover/controllers/discover.js @@ -546,8 +546,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(); @@ -833,8 +833,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/components/table_header.js b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header.js index 60d440b1f957d..e4f7f236f9023 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header.js +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header.js @@ -86,11 +86,12 @@ module.directive('kbnTableHeader', function () { $scope.headerClass = function (column) { if (!$scope.isSortableColumn(column)) return; - const sortOrder = $scope.sortOrder; - const defaultClass = ['fa', 'fa-sort-up', 'kbnDocTableHeader__sortChange']; + const defaultClass = ['fa', 'fa-sort', 'kbnDocTableHeader__sortChange']; + const sortOrder = $scope.sortOrder || []; + const columnSortOrder = sortOrder.find((sortPair) => column === sortPair[0]); - if (!sortOrder || column !== sortOrder[0]) return defaultClass; - return ['fa', sortOrder[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down']; + if (!columnSortOrder) return defaultClass; + return ['fa', columnSortOrder[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down']; }; $scope.moveColumnLeft = function moveLeft(columnName) { @@ -118,14 +119,23 @@ module.directive('kbnTableHeader', function () { return; } - const [currentColumnName, currentDirection = 'asc'] = $scope.sortOrder; - const newDirection = ( - (columnName === currentColumnName && currentDirection === 'asc') - ? 'desc' - : 'asc' - ); + const sortPair = $scope.sortOrder.find((pair) => pair[0] === columnName); - $scope.onChangeSortOrder(columnName, newDirection); + // Cycle goes Unsorted -> Asc -> Desc -> Unsorted + if (sortPair === undefined) { + $scope.onChangeSortOrder([[columnName, 'asc'], ...$scope.sortOrder]); + } + else if (sortPair[1] === 'asc') { + $scope.onChangeSortOrder([[columnName, 'desc'], ...$scope.sortOrder.filter((pair) => pair[0] !== columnName)]); + } + else if (sortPair[1] === 'desc' && $scope.sortOrder.length === 1) { + // 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. + $scope.onChangeSortOrder([[columnName, 'asc']]); + } + else { + $scope.onChangeSortOrder($scope.sortOrder.filter((pair) => pair[0] !== columnName)); + } }; $scope.getAriaLabelForColumn = function getAriaLabelForColumn(name) { 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 8591bb0ed7137..b6c3c86cdda99 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 @@ -43,11 +43,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; @@ -195,8 +195,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 }); }; @@ -257,6 +257,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; this.filtersSearchSource.setField('filter', this.input.filters); From 037bf03b699136644791316cb26e2b0245408eb1 Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Wed, 24 Jul 2019 16:58:35 -0400 Subject: [PATCH 2/9] Update functional test --- test/functional/apps/discover/_shared_links.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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( From 4e8bd681f37f48f2474443b6c5f5909eba09cc37 Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Wed, 24 Jul 2019 17:41:12 -0400 Subject: [PATCH 3/9] Fix row header tests --- .../doc_table/__tests__/lib/rows_headers.js | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/rows_headers.js b/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/rows_headers.js index b2660facf7a5a..d774f893615e5 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/rows_headers.js +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/__tests__/lib/rows_headers.js @@ -174,28 +174,29 @@ describe('Doc Table', 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']); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([[[SORTABLE_FIELDS[0], 'asc']]]); }); 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.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']); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([[[SORTABLE_FIELDS[0], 'asc']]]); }); - 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']; + it('should call onChangeSortOrder with ascending order for a sortable field in addition to an existing sort on a ' + + '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']); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([[[SORTABLE_FIELDS[0], 'asc'], [SORTABLE_FIELDS[1], 'asc']]]); }); 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.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']); + expect($scope.onChangeSortOrder.firstCall.args).to.eql([[[SORTABLE_FIELDS[0], 'desc']]]); }); it('should not call onChangeSortOrder for an unsortable field', function () { @@ -223,17 +224,17 @@ describe('Doc Table', function () { expect($scope.headerClass(UNSORTABLE_FIELDS[0])).to.be(undefined); }); - 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 return list including fa-sort for a sortable field not currently sorted by', function () { + expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort'); }); 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']; + $scope.sortOrder = [[SORTABLE_FIELDS[0], 'asc']]; expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-up'); }); 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']; + $scope.sortOrder = [[SORTABLE_FIELDS[0], 'desc']]; expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-down'); }); }); From 3312d7e05f311d078cdb816527e8e1bdf191343d Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Wed, 24 Jul 2019 18:49:06 -0400 Subject: [PATCH 4/9] Update getSort unit tests --- .../doc_table/__tests__/lib/get_sort.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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' ]]); }); }); }); From 8d1c62e1645c1f65e52a5d59502e3ac500a7173e Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Thu, 25 Jul 2019 13:41:10 -0400 Subject: [PATCH 5/9] reimplement multi sort in new react doc table header --- .../components/table_header/table_header.tsx | 7 +- .../table_header/table_header_column.tsx | 91 ++++++++++++++----- 2 files changed, 70 insertions(+), 28 deletions(-) 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 5700feeb71ad0..1919026a06799 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,38 +53,74 @@ 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 = () => { + // Cycle goes Unsorted -> Asc -> Desc -> Unsorted + if (currentColumnSort === undefined) { + onChangeSortOrder([[name, 'asc'], ...currentSortWithoutColumn]); + } else if (currentColumnSortDirection === 'asc') { + onChangeSortOrder([[name, 'desc'], ...currentSortWithoutColumn]); + } 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 getAriaLabel = () => { + 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, - 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: getAriaLabel(), 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}', From ff428a96aa69e851b3d378072e156c795b5032ac Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Thu, 25 Jul 2019 13:59:10 -0400 Subject: [PATCH 6/9] update jest tests --- .../doc_table/components/table_header/table_header.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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', () => { From 3ce8b536701602c19687320fea4206ba291297ed Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Fri, 26 Jul 2019 16:11:50 -0400 Subject: [PATCH 7/9] Fix type error introduce in merge --- .../doc_table/components/table_header/table_header_column.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx index da4a6b97586ff..d7e499cf38561 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx @@ -31,7 +31,7 @@ interface Props { isRemoveable: boolean; isSortable: boolean; name: string; - onChangeSortOrder: (sortOrder: SortOrder[]) => void; + onChangeSortOrder?: (sortOrder: SortOrder[]) => void; onMoveColumn?: (name: string, idx: number) => void; onRemoveColumn?: (name: string) => void; sortOrder: SortOrder[]; @@ -65,6 +65,8 @@ export function TableHeaderColumn({ sortDirection !== '' ? btnSortIcon : `kbnDocTableHeader__sortChange ${btnSortIcon}`; const handleChangeSortOrder = () => { + if (!onChangeSortOrder) return; + // Cycle goes Unsorted -> Asc -> Desc -> Unsorted if (currentColumnSort === undefined) { onChangeSortOrder([[name, 'asc'], ...currentSortWithoutColumn]); From 93d0ddb11d82fcc21e1e553df6ae6b570e46a6f7 Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Fri, 2 Aug 2019 10:59:21 -0400 Subject: [PATCH 8/9] Give sort button an active color and a more descriptive tooltip --- .../components/table_header/table_header_column.tsx | 9 +++------ .../ui/public/styles/_legacy/components/_table.scss | 4 ++-- x-pack/plugins/translations/translations/ja-JP.json | 1 - x-pack/plugins/translations/translations/zh-CN.json | 1 - 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx index d7e499cf38561..c84304ad9bb60 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx @@ -81,7 +81,7 @@ export function TableHeaderColumn({ } }; - const getAriaLabel = () => { + const getSortButtonAriaLabel = () => { const sortAscendingMessage = i18n.translate( 'kbn.docTable.tableHeader.sortByColumnAscendingAriaLabel', { @@ -120,14 +120,11 @@ export function TableHeaderColumn({ // Sort Button { active: isSortable && typeof onChangeSortOrder === 'function', - ariaLabel: getAriaLabel(), + ariaLabel: getSortButtonAriaLabel(), className: btnSortClassName, 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/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/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json index 0a2eab567d208..93a8e0e831ec8 100644 --- a/x-pack/plugins/translations/translations/ja-JP.json +++ b/x-pack/plugins/translations/translations/ja-JP.json @@ -1568,7 +1568,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 1b4dbfbf0d5d8..bc14e4220bb14 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -1568,7 +1568,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": "筛留值", From e4fd2a8f48869a10e59a079bfb946d37f5817eef Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Fri, 2 Aug 2019 11:08:23 -0400 Subject: [PATCH 9/9] Most recently clicked sort column will have lowest priority now --- .../doc_table/components/table_header/table_header_column.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx index c84304ad9bb60..87d0482053188 100644 --- a/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx +++ b/src/legacy/core_plugins/kibana/public/discover/doc_table/components/table_header/table_header_column.tsx @@ -69,9 +69,9 @@ export function TableHeaderColumn({ // Cycle goes Unsorted -> Asc -> Desc -> Unsorted if (currentColumnSort === undefined) { - onChangeSortOrder([[name, 'asc'], ...currentSortWithoutColumn]); + onChangeSortOrder([...currentSortWithoutColumn, [name, 'asc']]); } else if (currentColumnSortDirection === 'asc') { - onChangeSortOrder([[name, 'desc'], ...currentSortWithoutColumn]); + 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.