From 8eb17b330ec45626ab95f65965828b21beade840 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 11 Apr 2017 12:47:33 -0400 Subject: [PATCH] Refactor table sort properties (#11073) * Refactor sorting capabilities * clean up and comments * Move to ui_framework/services - add tests - address code review feedback - Include new folders in build output. * Rename to sortableProperties Require initial sorted property name. * fix a refactor miss --- .../dashboard/listing/dashboard_listing.html | 4 +- .../dashboard/listing/dashboard_listing.js | 37 ++---- .../visualize/listing/visualize_listing.html | 4 +- .../visualize/listing/visualize_listing.js | 83 ++++-------- src/jest/config.js | 3 + tasks/config/copy.js | 2 + ui_framework/services/index.js | 1 + ui_framework/services/sort/index.js | 1 + .../services/sort/sortable_properties.js | 99 ++++++++++++++ .../services/sort/sortable_properties.test.js | 122 ++++++++++++++++++ 10 files changed, 271 insertions(+), 85 deletions(-) create mode 100644 ui_framework/services/index.js create mode 100644 ui_framework/services/sort/index.js create mode 100644 ui_framework/services/sort/sortable_properties.js create mode 100644 ui_framework/services/sort/sortable_properties.test.js diff --git a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html index a07976d83ed2..dfa313582acd 100644 --- a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html +++ b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html @@ -121,11 +121,11 @@ > - + Name + ng-class="listingController.isAscending('title') ? 'fa-caret-up' : 'fa-caret-down'"> diff --git a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js index 85dddcf37d5b..4101fdd43e3a 100644 --- a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js +++ b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js @@ -1,8 +1,8 @@ import SavedObjectRegistryProvider from 'ui/saved_objects/saved_object_registry'; import 'ui/pager_control'; import 'ui/pager'; -import _ from 'lodash'; import { DashboardConstants, createDashboardEditUrl } from '../dashboard_constants'; +import { SortableProperties } from 'ui_framework/services'; export function DashboardListingController($injector, $scope) { const $filter = $injector.get('$filter'); @@ -21,21 +21,17 @@ export function DashboardListingController($injector, $scope) { const notify = new Notifier({ location: 'Dashboard' }); let selectedItems = []; - - /** - * Sorts hits either ascending or descending - * @param {Array} hits Array of saved finder object hits - * @return {Array} Array sorted either ascending or descending - */ - const sortItems = () => { - this.items = - this.isAscending - ? _.sortBy(this.items, 'title') - : _.sortBy(this.items, 'title').reverse(); - }; + const sortableProperties = new SortableProperties([ + { + name: 'title', + getValue: item => item.title, + isAscending: true, + } + ], + 'title'); const calculateItemsOnPage = () => { - sortItems(); + this.items = sortableProperties.sortItems(this.items); this.pager.setTotalItems(this.items.length); this.pageOfItems = limitTo(this.items, this.pager.pageSize, this.pager.startIndex); }; @@ -70,16 +66,11 @@ export function DashboardListingController($injector, $scope) { deselectAll(); fetchItems(); }); + this.isAscending = (name) => sortableProperties.isAscendingByName(name); + this.getSortedProperty = () => sortableProperties.getSortedProperty(); - /** - * Boolean that keeps track of whether hits are sorted ascending (true) - * or descending (false) by title - * @type {Boolean} - */ - this.isAscending = true; - - this.toggleSort = function toggleSort() { - this.isAscending = !this.isAscending; + this.sortOn = function sortOn(propertyName) { + sortableProperties.sortOn(propertyName); deselectAll(); calculateItemsOnPage(); }; diff --git a/src/core_plugins/kibana/public/visualize/listing/visualize_listing.html b/src/core_plugins/kibana/public/visualize/listing/visualize_listing.html index b14322308cc6..432e1f24081a 100644 --- a/src/core_plugins/kibana/public/visualize/listing/visualize_listing.html +++ b/src/core_plugins/kibana/public/visualize/listing/visualize_listing.html @@ -127,7 +127,7 @@ @@ -139,7 +139,7 @@ diff --git a/src/core_plugins/kibana/public/visualize/listing/visualize_listing.js b/src/core_plugins/kibana/public/visualize/listing/visualize_listing.js index d0bbd5bdf7d6..4c37c920e3a0 100644 --- a/src/core_plugins/kibana/public/visualize/listing/visualize_listing.js +++ b/src/core_plugins/kibana/public/visualize/listing/visualize_listing.js @@ -1,7 +1,8 @@ import SavedObjectRegistryProvider from 'ui/saved_objects/saved_object_registry'; import 'ui/pager_control'; import 'ui/pager'; -import _ from 'lodash'; + +import { SortableProperties } from 'ui_framework/services'; export function VisualizeListingController($injector, $scope) { const $filter = $injector.get('$filter'); @@ -20,23 +21,22 @@ export function VisualizeListingController($injector, $scope) { const notify = new Notifier({ location: 'Visualize' }); let selectedItems = []; - - /** - * Sorts hits either ascending or descending - * @param {Array} hits Array of saved finder object hits - * @return {Array} Array sorted either ascending or descending - */ - const sortItems = () => { - const sortProperty = this.getSortProperty(); - - this.items = - sortProperty.isAscending - ? _.sortBy(this.items, sortProperty.getValue) - : _.sortBy(this.items, sortProperty.getValue).reverse(); - }; + const sortableProperties = new SortableProperties([ + { + name: 'title', + getValue: item => item.title, + isAscending: true, + }, + { + name: 'type', + getValue: item => item.type.title, + isAscending: true, + } + ], + 'title'); const calculateItemsOnPage = () => { - sortItems(); + this.items = sortableProperties.sortItems(this.items); this.pager.setTotalItems(this.items.length); this.pageOfItems = limitTo(this.items, this.pager.pageSize, this.pager.startIndex); }; @@ -60,6 +60,15 @@ export function VisualizeListingController($injector, $scope) { selectedItems = this.pageOfItems.slice(0); }; + this.isAscending = (name) => sortableProperties.isAscendingByName(name); + this.getSortedProperty = () => sortableProperties.getSortedProperty(); + + this.sortOn = function sortOn(propertyName) { + sortableProperties.sortOn(propertyName); + deselectAll(); + calculateItemsOnPage(); + }; + this.isFetchingItems = false; this.items = []; this.pageOfItems = []; @@ -72,48 +81,6 @@ export function VisualizeListingController($injector, $scope) { fetchItems(); }); - /** - * Remember sort direction per property. - */ - this.sortProperties = [{ - name: 'title', - getValue: item => item.title, - isSelected: true, - isAscending: true, - }, { - name: 'type', - getValue: item => item.type.title, - isSelected: false, - isAscending: true, - }]; - - this.getSortProperty = function getSortProperty() { - return this.sortProperties.find(property => property.isSelected); - }; - - this.getSortPropertyByName = function getSortPropertyByName(name) { - return this.sortProperties.find(property => property.name === name); - }; - - this.isAscending = function isAscending() { - const sortProperty = this.getSortProperty(); - return sortProperty.isAscending; - }; - - this.sortOn = function sortOn(propertyName) { - const sortProperty = this.getSortProperty(); - - if (sortProperty.name === propertyName) { - sortProperty.isAscending = !sortProperty.isAscending; - } else { - sortProperty.isSelected = false; - this.getSortPropertyByName(propertyName).isSelected = true; - } - - deselectAll(); - calculateItemsOnPage(); - }; - this.toggleAll = function toggleAll() { if (this.areAllItemsChecked()) { deselectAll(); diff --git a/src/jest/config.js b/src/jest/config.js index c15b592ca605..a6eee669bad2 100644 --- a/src/jest/config.js +++ b/src/jest/config.js @@ -3,6 +3,9 @@ import { resolve } from 'path'; export const config = { roots: ['/ui_framework/'], collectCoverageFrom: [ + 'ui_framework/services/**/*.js', + '!ui_framework/services/index.js', + '!ui_framework/services/**/*/index.js', 'ui_framework/components/**/*.js', '!ui_framework/components/index.js', '!ui_framework/components/**/*/index.js', diff --git a/tasks/config/copy.js b/tasks/config/copy.js index f56880c123a5..22bc6f6d3eed 100644 --- a/tasks/config/copy.js +++ b/tasks/config/copy.js @@ -14,6 +14,8 @@ module.exports = function () { '!src/ui_framework/doc_site/**', '!src/es_archiver/**', 'bin/**', + 'ui_framework/components/**', + 'ui_framework/services/**', 'ui_framework/dist/**', 'webpackShims/**', 'config/kibana.yml', diff --git a/ui_framework/services/index.js b/ui_framework/services/index.js new file mode 100644 index 000000000000..287383f7e057 --- /dev/null +++ b/ui_framework/services/index.js @@ -0,0 +1 @@ +export { SortableProperties } from './sort'; diff --git a/ui_framework/services/sort/index.js b/ui_framework/services/sort/index.js new file mode 100644 index 000000000000..d945c274adff --- /dev/null +++ b/ui_framework/services/sort/index.js @@ -0,0 +1 @@ +export { SortableProperties } from './sortable_properties'; diff --git a/ui_framework/services/sort/sortable_properties.js b/ui_framework/services/sort/sortable_properties.js new file mode 100644 index 000000000000..f5069827dcb4 --- /dev/null +++ b/ui_framework/services/sort/sortable_properties.js @@ -0,0 +1,99 @@ +import _ from 'lodash'; + +/** + * @typedef {Object} SortableProperty + * @property {string} sortableProperty.name - Name of the property. + * @property {function} sortableProperty.getValue - A function that takes in an object and returns a value to sort + * by. + * @property {boolean} sortableProperty.isAscending - The direction of the last sort by this property. Used to preserve + * past sort orders. + */ + +/** + * Stores sort information for a set of SortableProperties, including which property is currently being sorted on, as + * well as the last sort order for each property. + */ +export class SortableProperties { + /** + * @param {Array} sortableProperties - a set of sortable properties. + * @param {string} initialSortablePropertyName - Which sort property should be sorted on by default. + */ + constructor(sortableProperties, initialSortablePropertyName) { + this.sortableProperties = sortableProperties; + /** + * The current property that is being sorted on. + * @type {SortableProperty} + */ + this.currentSortedProperty = this.getSortablePropertyByName(initialSortablePropertyName); + if (!this.currentSortedProperty) { + throw new Error(`No property with the name ${initialSortablePropertyName}`); + } + } + + /** + * @returns {SortableProperty} The current property that is being sorted on. Undefined if no sort order is applied. + */ + getSortedProperty() { + return this.currentSortedProperty; + } + + /** + * Sorts the items passed in and returns a newly sorted array. + * @param items {Array.} + * @returns {Array.} sorted array of items, based off the sort properties. + */ + sortItems(items) { + return this.isCurrentSortAscending() + ? _.sortBy(items, this.getSortedProperty().getValue) + : _.sortBy(items, this.getSortedProperty().getValue).reverse(); + } + + /** + * Returns the SortProperty with the given name, if found. + * @param {String} propertyName + * @returns {SortableProperty|undefined} + */ + getSortablePropertyByName(propertyName) { + return this.sortableProperties.find(property => property.name === propertyName); + } + + /** + * Updates the sort property, potentially flipping the sort order based on whether the same + * property was already being sorted. + * @param propertyName {String} + */ + sortOn(propertyName) { + const newSortedProperty = this.getSortablePropertyByName(propertyName); + const sortedProperty = this.getSortedProperty(); + if (sortedProperty.name === newSortedProperty.name) { + this.flipCurrentSortOrder(); + } else { + this.currentSortedProperty = newSortedProperty; + } + } + + /** + * @returns {boolean} True if the current sortable property is sorted in ascending order. + */ + isCurrentSortAscending() { + const sortedProperty = this.getSortedProperty(); + return sortedProperty ? this.isAscendingByName(sortedProperty.name) : false; + } + + /** + * @param {string} propertyName + * @returns {boolean} True if the given sort property is sorted in ascending order. + */ + isAscendingByName(propertyName) { + const sortedProperty = this.getSortablePropertyByName(propertyName); + return sortedProperty ? sortedProperty.isAscending : false; + } + + /** + * Flips the current sorted property sort order. + */ + flipCurrentSortOrder() { + this.currentSortedProperty.isAscending = !this.currentSortedProperty.isAscending; + } +} + diff --git a/ui_framework/services/sort/sortable_properties.test.js b/ui_framework/services/sort/sortable_properties.test.js new file mode 100644 index 000000000000..2d6c80aa0099 --- /dev/null +++ b/ui_framework/services/sort/sortable_properties.test.js @@ -0,0 +1,122 @@ +import _ from 'lodash'; +import { + SortableProperties, +} from './sortable_properties'; + +describe('SortProperties', () => { + const name = { + name: 'name', + getValue: bird => bird.name, + isAscending: true, + }; + + const size = { + name: 'size', + getValue: bird => bird.size, + isAscending: false, + }; + + const color = { + name: 'color', + getValue: bird => bird.color, + isAscending: true, + }; + + const birds = [ + { + name: 'cardinal', + color: 'red', + size: 7, + }, + { + name: 'bluejay', + color: 'blue', + size: 8, + }, + { + name: 'chickadee', + color: 'black and white', + size: 3, + } + ]; + + describe('initialSortProperty', () => { + test('is set', () => { + const sortableProperties = new SortableProperties([name, size, color], 'size'); + expect(sortableProperties.getSortedProperty().name).toBe('size'); + }); + + test('throws an error with an invalid property name', () => { + expect(() => new SortableProperties([name, size, color], 'does not exist')).toThrow(); + }); + + test('throws an error property name is not defined', () => { + expect(() => new SortableProperties([name, size, color])).toThrow(); + }); + }); + + describe('isAscendingByName', () => { + test('returns initial ascending values', () => { + const initialColorAscending = color.isAscending; + const initialNameAscending = name.isAscending; + const initialSizeAscending = size.isAscending; + + const sortableProperties = new SortableProperties([name, size, color], 'color'); + + expect(sortableProperties.isAscendingByName('color')).toBe(initialColorAscending); + expect(sortableProperties.isAscendingByName('name')).toBe(initialNameAscending); + expect(sortableProperties.isAscendingByName('size')).toBe(initialSizeAscending); + }); + }); + + describe('sortOn', () => { + test('a new property name retains the original sort order', () => { + const initialColorAscending = color.isAscending; + const sortableProperties = new SortableProperties([name, size, color], 'color'); + sortableProperties.sortOn('name'); + expect(sortableProperties.isAscendingByName('color')).toBe(initialColorAscending); + }); + + test('the same property name twice flips sort order', () => { + const initialColorAscending = color.isAscending; + const sortableProperties = new SortableProperties([name, size, color], 'color'); + sortableProperties.sortOn('color'); + expect(sortableProperties.isAscendingByName('color')).toBe(!initialColorAscending); + }); + }); + + describe('sortItems', () => { + test('sorts by initialSortProperty', () => { + const sortableProperties = new SortableProperties([name, size, color], 'name'); + const sortedItems = sortableProperties.sortItems(birds); + expect(sortedItems.length).toBe(birds.length); + expect(sortedItems[0].name).toBe('bluejay'); + expect(sortedItems[1].name).toBe('cardinal'); + expect(sortedItems[2].name).toBe('chickadee'); + }); + + test('first sorts by initial isAscending value', () => { + const sortableProperties = new SortableProperties([name, size, color], 'size'); + const sortedItems = sortableProperties.sortItems(birds); + expect(sortedItems[0].size).toBe(8); + expect(sortedItems[1].size).toBe(7); + expect(sortedItems[2].size).toBe(3); + }); + + test('sorts by descending', () => { + const sortableProperties = new SortableProperties([name, size, color], 'name'); + sortableProperties.sortOn('size'); + sortableProperties.sortOn('size'); + const sortedItems = sortableProperties.sortItems(birds); + expect(sortedItems[0].size).toBe(3); + expect(sortedItems[1].size).toBe(7); + expect(sortedItems[2].size).toBe(8); + }); + + test('empty items array is a noop', () => { + const sortableProperties = new SortableProperties([name, size, color], 'color'); + const sortedItems = sortableProperties.sortItems([]); + expect(sortedItems.length).toBe(0); + }); + }); +});