From 5d813bf07aa07d2e0ebb21722895d2fa0005604c Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 12 Jun 2018 16:45:20 -0600 Subject: [PATCH 1/4] Fix default sort comparator to logically handle null/undefined values --- src/services/sort/comparators.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/services/sort/comparators.js b/src/services/sort/comparators.js index bfc5759cd9a..b45295db79e 100644 --- a/src/services/sort/comparators.js +++ b/src/services/sort/comparators.js @@ -5,6 +5,34 @@ export const Comparators = Object.freeze({ default: (direction = SortDirection.ASC) => { return (v1, v2) => { + // JavaScript's comparison of null/undefined (and some others not handled here) values always returns `false` + // (https://www.ecma-international.org/ecma-262/#sec-abstract-relational-comparison) + // resulting in cases where v1 < v2 and v1 > v2 are both false. + // This leads to unpredictable sorting results in multiple JS engine sorting algorithms + // which causes unpredictable user experiences. + // Instead: + // * 1. push undefined/null values to the end of the sorted list, _regardless of sort direction_ + // (non-sortable values always appear at the end, and sortable values are sorted at the top) + // * 2. report undefined/null values as equal + // * 3. when both values are comparable they are sorted normally + + const v1IsComparable = v1 != null; + const v2IsComparable = v2 != null; + + // * 1. + if (v1IsComparable && !v2IsComparable) { + return -1; + } + if (!v1IsComparable && v2IsComparable) { + return 1; + } + + // * 2. + if (!v1IsComparable && !v2IsComparable) { + return 0; + } + + // * 3. if (v1 === v2) { return 0; } From a0de2053e3f8170759fabd30a31e2b49e90176e9 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 12 Jun 2018 17:01:49 -0600 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0112479698e..5776a1db6c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ **Bug fixes** - Added `role="dialog"` to `EuiFlyout` to improve screen reader accessibility ([#916](https://github.com/elastic/eui/pull/916)) +- Default sort comparator (used by `EuiInMemoryTable`) now handles `null` and `undefined` values ([#922](https://github.com/elastic/eui/pull/922)) ## [`0.0.52`](https://github.com/elastic/eui/tree/v0.0.52) From f080c102cb52bc51e53b017fb02557a0d0cfefb0 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 13 Jun 2018 09:36:38 -0600 Subject: [PATCH 3/4] Add unit test for sorting undefined values, modified docs data to include some empty values --- src-docs/src/views/tables/data_store.js | 2 +- src/services/sort/comparators.test.js | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src-docs/src/views/tables/data_store.js b/src-docs/src/views/tables/data_store.js index a1202e1faec..ab04a6cc379 100644 --- a/src-docs/src/views/tables/data_store.js +++ b/src-docs/src/views/tables/data_store.js @@ -32,7 +32,7 @@ const createCountries = () => [ ]; const firstNames = ['Very long first name that will wrap or be truncated', 'Another very long first name which will wrap or be truncated', - 'Clinton', 'Igor', 'Karl', 'Drew', 'Honza', 'Rashid', 'Jordan', 'John']; + 'Clinton', 'Igor', undefined, 'Drew', null, 'Rashid', undefined, 'John']; const lastNames = ['Very long last name that will wrap or be truncated', 'Another very long last name which will wrap or be truncated', 'Gormley', 'Motov', 'Minarik', 'Raines', 'Král', 'Khan', 'Sissel', 'Dorlus']; diff --git a/src/services/sort/comparators.test.js b/src/services/sort/comparators.test.js index 78bd16455bc..381fcdde753 100644 --- a/src/services/sort/comparators.test.js +++ b/src/services/sort/comparators.test.js @@ -46,3 +46,10 @@ describe('comparators - property', () => { }); }); +describe('default comparator', () => { + test('null/undefined values are sorted to the end', () => { + const sorted = [undefined, '7', 3, undefined, 5, undefined].sort(Comparators.default()); + expect(sorted).toEqual([3, 5, '7', undefined, undefined, undefined]) + }) +}); + From efef2fdef5fde494e7be98a55ed242ec69c668ed Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 13 Jun 2018 12:23:22 -0600 Subject: [PATCH 4/4] update default comparator test to include a null value --- src/services/sort/comparators.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/sort/comparators.test.js b/src/services/sort/comparators.test.js index 381fcdde753..5e59d187b67 100644 --- a/src/services/sort/comparators.test.js +++ b/src/services/sort/comparators.test.js @@ -48,8 +48,8 @@ describe('comparators - property', () => { describe('default comparator', () => { test('null/undefined values are sorted to the end', () => { - const sorted = [undefined, '7', 3, undefined, 5, undefined].sort(Comparators.default()); - expect(sorted).toEqual([3, 5, '7', undefined, undefined, undefined]) + const sorted = [undefined, '7', 3, null, 5, undefined].sort(Comparators.default()); + expect(sorted).toEqual([3, 5, '7', null, undefined, undefined]) }) });