From cda5b21471f10342eac260b84de0bc31126bd467 Mon Sep 17 00:00:00 2001 From: Mike Wislek <49659689+mwislek@users.noreply.github.com> Date: Fri, 24 Jul 2020 09:33:28 -0500 Subject: [PATCH] Refactor table sort (#437) * Initial refactor or SortableTable to not use sortable() HOC * Refactored value getter * Added stories and tests for initial column sort * Update version * Updates with review comments * Additional review comments * Trigger travis Co-authored-by: David Pickart --- package.json | 2 +- src/tables/components/table-row.js | 4 +- src/tables/helpers/index.js | 2 +- src/tables/sortable-table.js | 170 +++++++++++++++---------- src/utils/index.js | 1 + stories/tables/sortable-table.story.js | 114 ++++++++++++----- test/tables/sortable-table.test.js | 144 ++++++++++++++------- 7 files changed, 290 insertions(+), 147 deletions(-) diff --git a/package.json b/package.json index b4f1470f..6aebf654 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@launchpadlab/lp-components", - "version": "3.32.0", + "version": "3.32.1", "engines": { "node": "^8.0.0 || ^10.13.0 || ^12.0.0" }, diff --git a/src/tables/components/table-row.js b/src/tables/components/table-row.js index 4f753f1e..9402ee7a 100644 --- a/src/tables/components/table-row.js +++ b/src/tables/components/table-row.js @@ -25,11 +25,11 @@ function TableRow ({ const { name, component: CellComponent=DefaultCellComponent, format=identity, onClick=noop, valueGetter, ...rest } = column const cellValue = valueGetter ? valueGetter(rowData) : get(name, rowData) - const value = format(cellValue) + const formattedValue = format(cellValue) const onColClick = column.disabled ? noop : () => onClick(rowData) return col.name === initialColumn).pop() + // Exceptional situation-- an initial column was specified but no column data + // exists for the named column... + if (!initialProps) throw new Error('initial column has no column definition') + + return { + initialSortPath: initialProps.name, + initialSortFunc: initialProps.sortFunc, + initialValueGetter: initialProps.valueGetter, + } +} + -function SortableTable ({ - columns, +function SortableTable({ + className, + children, data: unsortedData, + initialAscending, + initialColumn, + disableReverse, disableSort, controlled, - sort, - ascending, - sortPath, - setSortPath, - setSortFunc, + onChange, rowComponent, headerComponent, - className, }) { - const data = (controlled || disableSort) ? unsortedData : sort(unsortedData) + const columns = getColumnData(children, disableSort) + const { initialSortPath, initialSortFunc, initialValueGetter } = + getInitialSortControls(initialColumn, columns) + const [ascending, setAscending] = useState(initialAscending) + const [sortPath, setSortPath] = useState(initialSortPath) + + // Setting and storing a function object requires special syntax. + // See: https://medium.com/swlh/how-to-store-a-function-with-the-usestate-hook-in-react-8a88dd4eede1 + const [sortFunc, setSortFunc] = useState(() => initialSortFunc) + const [valueGetter, setValueGetter] = useState(() => initialValueGetter) + + const data = useMemo(() => { + if (controlled || disableSort) return unsortedData + + if (sortFunc) { + const sorted = [...unsortedData].sort(sortFunc) + if (!ascending && !disableReverse) sorted.reverse() + return sorted + } + else { + const order = ascending ? 'asc' : 'desc' + const sorted = orderBy( + unsortedData, + (item) => valueGetter ? valueGetter(item) : get(sortPath, item), + order + ) + return sorted + } + }, [ascending, sortPath, sortFunc, valueGetter]) + + const handleColumnChange = (column) => { + if (column.disabled) return + + const newSortPath = column.name + const newSortFunc = column.sortFunc || null + const newValueGetter = column.valueGetter || null + + // Toggle ascending if the path is already selected. Otherwise, default + // to ascending when switching paths... + const newAscending = newSortPath === sortPath ? !ascending : true + + setAscending(newAscending) + setSortPath(newSortPath) + setSortFunc(() => newSortFunc) + setValueGetter(() => newValueGetter) + + if (onChange) onChange({ + ascending: newAscending, + sortPath: newSortPath, + sortFunc: newSortFunc + }) + } + return ( - +
{ columns.map((column, key) => { @@ -77,13 +158,7 @@ function SortableTable ({ column, sortPath, ascending, - onClick: () => { - if (column.disabled) return - const newSortPath = column.name - const newSortFunc = column.sortFunc || null - setSortPath(newSortPath) - setSortFunc(newSortFunc) - } + onClick: () => handleColumnChange(column) }} /> ) } @@ -109,43 +184,4 @@ function SortableTable ({ SortableTable.propTypes = propTypes SortableTable.defaultProps = defaultProps -const WrappedTable = sortable()(SortableTable) - -// Passes relevant sortable props -function SortableTableWrapper ({ initialColumn, children, disableSort, disableReverse, onChange, ...rest }) { - const columns = getColumnData(children, disableSort) - const initialProps = columns.filter(col => col.name === initialColumn).pop() - return -} - -SortableTableWrapper.propTypes = { - initialColumn: PropTypes.string, - children: PropTypes.node.isRequired, - data: PropTypes.arrayOf(PropTypes.object), - disableSort: PropTypes.bool, - disableReverse: PropTypes.bool, - onChange: PropTypes.func, - rowComponent: Types.component, - headerComponent: Types.component, -} - -SortableTableWrapper.defaultProps = { - initialColumn: '', - disableSort: false, - controlled: false, - disableReverse: false, - data: [], - onChange: noop, -} - -export default SortableTableWrapper +export default SortableTable diff --git a/src/utils/index.js b/src/utils/index.js index bbf0bff0..c03c37b1 100644 --- a/src/utils/index.js +++ b/src/utils/index.js @@ -6,6 +6,7 @@ export { startCase, range, noop, + orderBy, first, toLower, union as addToArray, diff --git a/stories/tables/sortable-table.story.js b/stories/tables/sortable-table.story.js index 558f7482..94d41887 100644 --- a/stories/tables/sortable-table.story.js +++ b/stories/tables/sortable-table.story.js @@ -1,7 +1,7 @@ import React from 'react' import { storiesOf } from '@storybook/react' import { lowerCase } from 'lodash' -import { SortableTable, TableColumn as Column } from 'src' +import { SortableTable, TableColumn as Column, compareAtPath } from 'src' const tableData = [ { name: 'Kim', age: 45, active: 'yes' }, @@ -9,28 +9,28 @@ const tableData = [ { name: 'Lorax', age: 450, active: 'yes' }, ] -function colorForStatus (active) { +function colorForStatus(active) { return active === 'yes' ? 'green' : 'red' } // eslint-disable-next-line react/prop-types -function CustomCell ({ value }) { +function CustomCell({ value }) { return ( - + ) } // eslint-disable-next-line react/prop-types -function CustomRow ({ data: { active }, children }) { +function CustomRow({ data: { active }, children }) { return ( - { children } + {children} ) } // eslint-disable-next-line react/prop-types -function CustomHeader ({ column: { name } }) { +function CustomHeader({ column: { name } }) { return ( - + ) } @@ -38,9 +38,16 @@ function createCustomValue(data) { return `${data.name}:${data.age}` } +function compareCustomValue(a, b) { + const ageA = Number(a) + const ageB = Number(b) + + return (ageA > ageB) ? 1 : -1 +} + storiesOf('SortableTable', module) .add('default', () => ( - + @@ -49,44 +56,58 @@ storiesOf('SortableTable', module) .add('with initial column', () => (

Table is sorted by "age" by default.

- + - + + + +
+ )) + .add('with initial column sorted descending', () => ( +
+

Table is sorted (descending) by "age" by default.

+ + +
)) .add('with custom labels', () => ( - + )) .add('with custom cell component', () => ( - + - + )) .add('with custom row component', () => ( - + )) .add('with custom header component', () => ( - + )) .add('with column-specific custom header component', () => ( - - + + @@ -94,25 +115,58 @@ storiesOf('SortableTable', module) .add('with disabled column', () => (

The "age" column is disabled and will not sort.

- + - +
)) .add('with formatted column values', () => ( - - - val.toFixed(1) } /> - val === 'yes' ? 'Y' : 'N' } /> + + + val.toFixed(1)} /> + val === 'yes' ? 'Y' : 'N'} /> )) .add('with custom value getter', () => ( - - - val.toFixed(1) } /> - val === 'yes' ? 'Y' : 'N' } /> - - +
+

"Name And Age" column combines name and age values

+ + + val.toFixed(1)} /> + val === 'yes' ? 'Y' : 'N'} /> + + +
+ )) + .add('with custom value getter and custom sorter', () => ( +
+

"Name and Age" column combines name and age, sorted by age portion

+ + + val.toFixed(1)} /> + val === 'yes' ? 'Y' : 'N'} /> + + +
+ )) + .add('with custom value getter, custom sorter, iniital column', () => ( +
+

"Name and Age" column combines name and age, sorted by age portion, initial column

+ + + val.toFixed(1)} /> + val === 'yes' ? 'Y' : 'N'} /> + + +
)) diff --git a/test/tables/sortable-table.test.js b/test/tables/sortable-table.test.js index d6c2b2ca..01dc6dc4 100644 --- a/test/tables/sortable-table.test.js +++ b/test/tables/sortable-table.test.js @@ -13,8 +13,8 @@ const sortAscending = (a, b) => (a > b) ? 1 : -1 test('Column data is pulled out via name', () => { const wrapper = mount( - - + + ) expect(wrapper.find('td').first().text()).toEqual('Kim') @@ -23,9 +23,9 @@ test('Column data is pulled out via name', () => { test('Columns without props are ignored', () => { const wrapper = mount( - - - { false } + + + {false} ) expect(wrapper.find('td').first().text()).toEqual('Kim') @@ -34,8 +34,8 @@ test('Columns without props are ignored', () => { test('Clicking on column header changes sortPath', () => { const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -47,8 +47,8 @@ test('Clicking on column header changes sortPath', () => { test('onChange is fired when sorting state changes', () => { const onChange = jest.fn() const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -58,8 +58,8 @@ test('onChange is fired when sorting state changes', () => { test('Clicking on column header twice toggles ascending', () => { const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -71,8 +71,8 @@ test('Clicking on column header twice toggles ascending', () => { test('Clicking on disabled column header does nothing', () => { const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -83,8 +83,8 @@ test('Clicking on disabled column header does nothing', () => { test('disableSort disables all columns', () => { const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -95,8 +95,8 @@ test('disableSort disables all columns', () => { test('controlled disables all columns', () => { const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -107,8 +107,8 @@ test('controlled disables all columns', () => { test('column can have custom label', () => { const wrapper = mount( - - + + ) expect(wrapper.find('th').first().text()).toEqual('FOO') @@ -117,8 +117,8 @@ test('column can have custom label', () => { test('column can have custom sort function', () => { const mySort = jest.fn(compareAtPath('name', sortAscending)) const wrapper = mount( - - + + ) wrapper.find('th').first().simulate('click') @@ -127,8 +127,8 @@ test('column can have custom sort function', () => { test('column can have custom className', () => { const wrapper = mount( - - + + ) expect(wrapper.find('td.foo').exists()).toBe(true) @@ -137,8 +137,8 @@ test('column can have custom className', () => { test('column can have custom cell component', () => { const MyCell = () => const wrapper = mount( - - + + ) expect(wrapper.find(MyCell).exists()).toBe(true) @@ -152,10 +152,10 @@ test('column can have custom cell component', () => { }) test('column can have custom row component', () => { - const MyRow = ({ children }) => { children } // eslint-disable-line + const MyRow = ({ children }) => {children} // eslint-disable-line const wrapper = mount( - - + + ) expect(wrapper.find(MyRow).exists()).toBe(true) @@ -163,11 +163,11 @@ test('column can have custom row component', () => { }) test('column can have custom header component', () => { - const MyHeader = ({ column: { name } }) => // eslint-disable-line + const MyHeader = ({ column: { name } }) => // eslint-disable-line const wrapper = mount( - - - + + + ) expect(wrapper.find(MyHeader).exists()).toBe(true) @@ -177,11 +177,11 @@ test('column can have custom header component', () => { }) test('column can have a column-specific custom header component', () => { - const MyHeader = ({ column: { name } }) => // eslint-disable-line + const MyHeader = ({ column: { name } }) => // eslint-disable-line const wrapper = mount( - - - + + + ) expect(wrapper.find(MyHeader).exists()).toBe(true) @@ -192,8 +192,8 @@ test('column can have a column-specific custom header component', () => { test('initialColumn determines inital sortPath and sortFunc', () => { const mySort = jest.fn(compareAtPath('name', sortAscending)) const wrapper = mount( - - + + ) // Data should now be sorted by name @@ -202,11 +202,26 @@ test('initialColumn determines inital sortPath and sortFunc', () => { expect(mySort).toHaveBeenCalled() }) +test('initialColumn can be default sorted descending', () => { + const wrapper = mount( + + + + ) + // Data should now be sorted, descending, by name + expect(wrapper.find('td').first().text()).toEqual('Tommy') + expect(wrapper.find('td').last().text()).toEqual('Kim') +}) + test('`onClick` function is called on correct column cells', () => { const onClick = jest.fn() const wrapper = mount( - - + + ) @@ -221,8 +236,8 @@ test('`onClick` function is called on correct column cells', () => { test('`format` updates the cell value', () => { const format = jest.fn(lowerCase) const wrapper = mount( - - + + ) expect(wrapper.find('td').first().text()).toEqual('kim') @@ -236,7 +251,7 @@ test('`placeholder` option is displayed if value is `null` or `undefined`', () = { name: undefined }, ] const wrapper = mount( - + ) @@ -250,7 +265,7 @@ test('can recieve custom class name', () => { { name: undefined }, ] const wrapper = mount( - + ) @@ -264,11 +279,48 @@ test('`valueGetter` derives the cell value', () => { ] const myValueGetter = jest.fn((data) => `${data.name} - ${data.accountName}`) const wrapper = mount( - - + + ) expect(wrapper.find('td').first().text()).toEqual('Opportunity 1 - Dealer 1') expect(wrapper.find('td').last().text()).toEqual('Opportunity 2 - Dealer 2') expect(myValueGetter).toHaveBeenCalled() }) + +test('`valueGetter` can utilize the default sort', () => { + const data = [ + { name: 'Opportunity 2', accountName: 'Dealer 2' }, + { name: 'Opportunity 1', accountName: 'Dealer 1' }, + ] + const myValueGetter = jest.fn((data) => `${data.name} - ${data.accountName}`) + const wrapper = mount( + + + + ) + wrapper.find('th').first().simulate('click') + // Data should now be sorted by derived data values + expect(wrapper.find('td').first().text()).toEqual('Opportunity 1 - Dealer 1') + expect(wrapper.find('td').last().text()).toEqual('Opportunity 2 - Dealer 2') + + expect(myValueGetter).toHaveBeenCalled() +}) + +test('`valueGetter` column can be the initial column and is sorted ascending', () => { + const data = [ + { name: 'Opportunity 2', accountName: 'Dealer 2' }, + { name: 'Opportunity 1', accountName: 'Dealer 1' }, + ] + const myValueGetter = jest.fn((data) => `${data.name} - ${data.accountName}`) + const wrapper = mount( + + + + + + ) + expect(wrapper.find('td').first().text()).toEqual('Opportunity 1 - Dealer 1') + expect(myValueGetter).toHaveBeenCalled() +}) +
{ value }{value}
{ name.toUpperCase() + '!' }{name.toUpperCase() + '!'} Hi!
{ name }{name}{ name }{name}