From 00091a71cfcb808295040c7d460c3db64bad1037 Mon Sep 17 00:00:00 2001 From: flannanl Date: Thu, 9 May 2019 10:20:52 -0400 Subject: [PATCH] fix(filters): do not sort the selected items to the top of the list (#80) --- .../NestedFilterableMultiselect-test.js | 41 +++++++------- .../tools/__tests__/sorting-test.js | 56 ------------------- src/components/MultiSelect/tools/sorting.js | 17 +----- 3 files changed, 23 insertions(+), 91 deletions(-) diff --git a/src/components/MultiSelect/__tests__/NestedFilterableMultiselect-test.js b/src/components/MultiSelect/__tests__/NestedFilterableMultiselect-test.js index cc59a5f..05f8b8e 100644 --- a/src/components/MultiSelect/__tests__/NestedFilterableMultiselect-test.js +++ b/src/components/MultiSelect/__tests__/NestedFilterableMultiselect-test.js @@ -135,7 +135,7 @@ describe('NestedFilterableMultiselect', () => { wrapper .find(listItemName) - .at(0) + .at(1) .simulate('click'); expect(mockProps.onChange).toHaveBeenCalledTimes(4); expect(mockProps.onChange).toHaveBeenCalledWith({ @@ -615,7 +615,7 @@ describe('NestedFilterableMultiselect', () => { wrapper .find('.bx--checkbox-label') - .at(0) + .at(1) .simulate('click'); expect(mockProps.onChange).toHaveBeenCalledTimes(4); expect(mockProps.onChange).toHaveBeenCalledWith({ @@ -634,6 +634,7 @@ describe('NestedFilterableMultiselect', () => { .at(0) .find('span') .simulate('click'); + // Check the first suboption wrapper .find('.bx--checkbox-label') .at(1) @@ -659,6 +660,7 @@ describe('NestedFilterableMultiselect', () => { ).toBe(true); expect(wrapper.find('ListBoxSelection').prop('selectionCount')).toBe(1); + // Check the second suboption wrapper .find('.bx--checkbox-label') .at(2) @@ -683,7 +685,8 @@ describe('NestedFilterableMultiselect', () => { .prop('indeterminate') ).toBe(false); expect(wrapper.find('ListBoxSelection').prop('selectionCount')).toBe(2); - // Un-select the child items + + // Un-select the first suboption wrapper .find('.bx--checkbox-label') .at(1) @@ -708,9 +711,10 @@ describe('NestedFilterableMultiselect', () => { ).toBe(true); expect(wrapper.find('ListBoxSelection').prop('selectionCount')).toBe(1); + // Un-select the second suboption wrapper .find('.bx--checkbox-label') - .at(1) + .at(2) .simulate('click'); expect(mockProps.onChange).toHaveBeenCalledTimes(4); expect(mockProps.onChange).toHaveBeenCalledWith({ @@ -914,14 +918,13 @@ describe('NestedFilterableMultiselect', () => { .at(1) .simulate('click'); expect(mockProps.onChange).toHaveBeenCalledTimes(1); - // checked item is now at the top expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('indeterminate') ).toBe(false); // expand suboptions wrapper .find('.bx--checkbox-label') - .at(0) + .at(1) .find('span') .simulate('click'); expect( @@ -930,7 +933,7 @@ describe('NestedFilterableMultiselect', () => { // unselect subOption wrapper .find('.bx--checkbox-label') - .at(2) + .at(3) .simulate('click'); expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('indeterminate') @@ -938,7 +941,7 @@ describe('NestedFilterableMultiselect', () => { // expand subChild wrapper .find('.bx--checkbox-label') - .at(1) + .at(2) .find('span') .simulate('click'); expect( @@ -947,7 +950,7 @@ describe('NestedFilterableMultiselect', () => { // unselect subChild wrapper .find('.bx--checkbox-label') - .at(2) + .at(3) .simulate('click'); expect( wrapper.find('Checkbox[name="Sub item 1"]').prop('indeterminate') @@ -970,16 +973,16 @@ describe('NestedFilterableMultiselect', () => { expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('checked') ).toBe(true); - // checked item is now at the top, expand suboptions + // expand suboptions wrapper .find('.bx--checkbox-label') - .at(0) + .at(1) .find('span') .simulate('click'); // unselect 1 subOption wrapper .find('.bx--checkbox-label') - .at(1) + .at(2) .simulate('click'); expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('indeterminate') @@ -990,7 +993,7 @@ describe('NestedFilterableMultiselect', () => { // unselect 2 subOption wrapper .find('.bx--checkbox-label') - .at(1) + .at(3) .simulate('click'); expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('checked') @@ -1010,16 +1013,16 @@ describe('NestedFilterableMultiselect', () => { expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('checked') ).toBe(true); - // checked item is now at the top, expand suboptions + // expand suboptions wrapper .find('.bx--checkbox-label') - .at(0) + .at(1) .find('span') .simulate('click'); // unselect 1 subOption wrapper .find('.bx--checkbox-label') - .at(2) + .at(3) .simulate('click'); expect( wrapper.find('Checkbox[name="Nested item 2"]').prop('indeterminate') @@ -1030,13 +1033,13 @@ describe('NestedFilterableMultiselect', () => { // expand subChild wrapper .find('.bx--checkbox-label') - .at(1) + .at(2) .find('span') .simulate('click'); // unselect 1 subChild wrapper .find('.bx--checkbox-label') - .at(2) + .at(3) .simulate('click'); expect( wrapper.find('Checkbox[name="Sub item 1"]').prop('indeterminate') @@ -1053,7 +1056,7 @@ describe('NestedFilterableMultiselect', () => { // unselect 2 subChild wrapper .find('.bx--checkbox-label') - .at(2) + .at(4) .simulate('click'); expect( wrapper.find('Checkbox[name="Sub item 1"]').prop('indeterminate') diff --git a/src/components/MultiSelect/tools/__tests__/sorting-test.js b/src/components/MultiSelect/tools/__tests__/sorting-test.js index 4ccc00a..ae89923 100644 --- a/src/components/MultiSelect/tools/__tests__/sorting-test.js +++ b/src/components/MultiSelect/tools/__tests__/sorting-test.js @@ -89,62 +89,6 @@ describe('defaultSortItems', () => { ]); }); - it('should order a selected item before all other options', () => { - const mockItems = ['Option 1', 'Option 10', 'Option 11', 'Option 2'].map( - label => ({ id: label, label }) - ); - - // Set `selectedItems` to ['Option 11'] - mockOptions.selectedItems = [mockItems[2]]; - - expect(defaultSortItems(mockItems, mockOptions)).toEqual([ - { - id: 'Option 11', - label: 'Option 11', - }, - { - id: 'Option 1', - label: 'Option 1', - }, - { - id: 'Option 2', - label: 'Option 2', - }, - { - id: 'Option 10', - label: 'Option 10', - }, - ]); - }); - - it('should sort selected items and order them before all other options', () => { - const mockItems = ['Option 1', 'Option 10', 'Option 11', 'Option 2'].map( - label => ({ id: label, label }) - ); - - // Set `selectedItems` to ['Option 11', 'Option 2'] - mockOptions.selectedItems = [mockItems[2], mockItems[3]]; - - expect(defaultSortItems(mockItems, mockOptions)).toEqual([ - { - id: 'Option 2', - label: 'Option 2', - }, - { - id: 'Option 11', - label: 'Option 11', - }, - { - id: 'Option 1', - label: 'Option 1', - }, - { - id: 'Option 10', - label: 'Option 10', - }, - ]); - }); - it('should sort parent and child', () => { const mockItems = [ { diff --git a/src/components/MultiSelect/tools/sorting.js b/src/components/MultiSelect/tools/sorting.js index fce9385..320eb3b 100644 --- a/src/components/MultiSelect/tools/sorting.js +++ b/src/components/MultiSelect/tools/sorting.js @@ -55,13 +55,10 @@ export const defaultCompareItems = (itemA, itemB, { locale }) => */ export const defaultSortItems = ( items, - { selectedItems, itemToString, compareItems, locale = 'en' } + { itemToString, compareItems, locale = 'en' } ) => { const itemArr = [...items]; return items.sort((itemA, itemB) => { - const hasItemA = selectedItems.some(item => item.id === itemA.id); - const hasItemB = selectedItems.some(item => item.id === itemB.id); - const hierarchyA = buildHierarchy(itemA, itemArr); const hierarchyB = buildHierarchy(itemB, itemArr); const depth = @@ -85,18 +82,6 @@ export const defaultSortItems = ( return -1; } - const hasCurrentA = selectedItems.some(item => item.id === currentA.id); - const hasCurrentB = selectedItems.some(item => item.id === currentB.id); - - // Prefer whichever item is in the `selectedItems` array first - if (hasCurrentA && !hasCurrentB) { - return -1; - } - - if (hasCurrentB && !hasCurrentA) { - return 1; - } - compareResult = compareItems( itemToString(currentA), itemToString(currentB),