Refactor table sort properties #11073

stacey-gammon · 2017-04-06T11:46:08Z

Pushes common sort logic from visualize and dashboard landing pages into a common file. I want to add a new column to dashboard, and currently the logic is not in place for it to sort on multiple columns - the logic assumes only one column. So I tackled this refactor first.

cjcenizal

Awesome! Just had a couple requests.

cjcenizal · 2017-04-06T19:14:47Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing.js

@@ -60,6 +60,15 @@ export function VisualizeListingController($injector, $scope) {
    selectedItems = this.pageOfItems.slice(0);
  };

+  this.isAscending = () => sortProperties.isAscending();


I think it's worth future-proofing this a bit by changing it so that it accepts a property name and returns the ascending value of that SortProperty. The only reason this currently works from a UI perspective is because the sort arrow gets hidden if a column isn't being sorted on. If we ever decide to show that arrow for columns that aren't being sorted on (e.g. maybe we show it on hover), then the direction would be incorrect.

cjcenizal · 2017-04-06T19:17:49Z

src/ui/public/sort/sort_properties.js

+ * Stores sort information for a set of SortProperties, including which property is currently being sorted on, as
+ * well as the last sort order for each property.
+ */
+export class SortProperties {


Can we add tests for this file? Should be pretty straightforward.

Whoa I could have sworn I added a comment that said I was going to add tests as soon as you approved of the general direction. :) Just wanted to make sure we were on the same page first, in case there was conflict and the pr got scrapped.

cjcenizal · 2017-04-06T22:34:03Z

BTW, I think at some point we should add a ui_framework/services directory, which will house non-visual yet UI-specific logic like sort_properties.js. This will help codify the behavior of our components. As an added benefit, we can write the tests for these services in Jest and they'll be part of our coverage report.

For reference, here is a similar thing I did with the UI framework at my previous job: https://github.com/smaato/ui-framework/tree/develop/src/framework/services. It contained services for sorting, filtering, scrolling, handling key input, and humanizing numbers. There was also a test utility which looks very similar to something you wrote in another PR. 😄

In fact, if you're planning on writing tests, maybe we should create this directory as part of this PR, so we don't have to rewrite the tests later?

stacey-gammon · 2017-04-06T22:37:11Z

I love that idea, happy to move it over. Was struggling with where to put that kind of logic. ++

stacey-gammon · 2017-04-07T18:26:40Z

@cjcenizal, ready to go for look two, with tests and the new folder location!

cjcenizal

Hahaha sweeeeet!!!! It's so cool seeing these tests included in the coverage report. I just had a couple suggestions.

cjcenizal · 2017-04-07T19:48:08Z

ui_framework/services/sort/sort_properties.test.js

+      expect(new SortProperties([name, size, color], 'does not exist')).toThrow();
+    });
+
+    test('is optional', () => {


Maybe this shouldn't be optional. Should we throw an error if it isn't supplied? I think we always want to set a default sort for anything that can be sorted.

cjcenizal · 2017-04-07T19:51:08Z

ui_framework/services/sort/sort_properties.test.js

+      let sortedItems = sortProperties.sortItems(birds);
+      expect(sortedItems[0].size).toBe(8);
+      expect(sortedItems[1].size).toBe(7);
+      expect(sortedItems[2].size).toBe(3);


I don't think we need these first three assertions since they're already covered by the preceding test.

cjcenizal · 2017-04-07T19:53:33Z

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 { SortProperties } from 'ui_framework/services';


I think we're going to find ourselves with a lot of services that are standalone functions, which will all have verb-based names. Since "sort" is also a verb, I think this name is a bit ambiguous. How about renaming it to SortableProperties?

good point, sortableProperties is more accurate. ✅

- add tests - address code review feedback - Include new folders in build output.

Require initial sorted property name.

stacey-gammon · 2017-04-11T15:50:35Z

Just ran into this again #11040:

>> FAIL: chrome on any platform - kibana - settings app - creating and using Painless boolean scripted fields - should visualize scripted field in vertical bar chart (3706ms)
Error: expected [ 'true 359' ] to sort of equal [ 'true 359', 'false 27' ]

  [
    0: "true 359",
E   1: "false 27",
E   length: 2
A   length: 1
  ]

Jenkins, test this

cjcenizal

LGTM!

* 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

stacey-gammon requested a review from cjcenizal April 6, 2017 12:59

stacey-gammon added Feature:Dashboard Dashboard related features :Sharing review labels Apr 6, 2017

cjcenizal suggested changes Apr 6, 2017

View reviewed changes

cjcenizal suggested changes Apr 7, 2017

View reviewed changes

stacey-gammon force-pushed the refactor-landing-page-sorting branch 2 times, most recently from d629e72 to edfc938 Compare April 10, 2017 11:13

stacey-gammon added 5 commits April 11, 2017 10:06

Refactor sorting capabilities

2339011

clean up and comments

33afc7e

Move to ui_framework/services

dcfdfee

- add tests - address code review feedback - Include new folders in build output.

Rename to sortableProperties

cc633a2

Require initial sorted property name.

fix a refactor miss

d91df10

stacey-gammon force-pushed the refactor-landing-page-sorting branch from edfc938 to d91df10 Compare April 11, 2017 14:59

cjcenizal approved these changes Apr 11, 2017

View reviewed changes

stacey-gammon merged commit 8eb17b3 into elastic:master Apr 11, 2017

stacey-gammon deleted the refactor-landing-page-sorting branch April 13, 2017 13:07

stacey-gammon added the v5.5.0 label May 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor table sort properties #11073

Refactor table sort properties #11073

stacey-gammon commented Apr 6, 2017

cjcenizal left a comment

cjcenizal Apr 6, 2017

cjcenizal Apr 6, 2017

stacey-gammon Apr 6, 2017

cjcenizal Apr 6, 2017

cjcenizal commented Apr 6, 2017 •

edited

Loading

stacey-gammon commented Apr 6, 2017

stacey-gammon commented Apr 7, 2017

cjcenizal left a comment

cjcenizal Apr 7, 2017

cjcenizal Apr 7, 2017

cjcenizal Apr 7, 2017

stacey-gammon Apr 7, 2017

stacey-gammon commented Apr 11, 2017

cjcenizal left a comment

Refactor table sort properties #11073

Refactor table sort properties #11073

Conversation

stacey-gammon commented Apr 6, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Apr 6, 2017 • edited Loading

stacey-gammon commented Apr 6, 2017

stacey-gammon commented Apr 7, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Apr 11, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal commented Apr 6, 2017 •

edited

Loading