Skip to content

Commit

Permalink
[PR feedback] Rename fns to indicate multiple concerns
Browse files Browse the repository at this point in the history
- having a side effect in a getter feels bad, so change that to a `find`
- rename use hook to indicate sorting and pagination concerns
  • Loading branch information
cee-chen committed Feb 1, 2022
1 parent 56465f6 commit 55a4ba6
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 28 deletions.
40 changes: 20 additions & 20 deletions src/components/datagrid/utils/ref.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { testCustomHook } from '../../../test/test_custom_hook.test_helper';
import { useCellLocationCheck, useVisibleRowIndex } from './ref';
import { useCellLocationCheck, useSortPageCheck } from './ref';

// see ref.spec.tsx for E2E useImperativeGridRef tests

Expand Down Expand Up @@ -39,17 +39,17 @@ describe('useCellLocationCheck', () => {
});
});

describe('useVisibleRowIndex', () => {
describe('useSortPageCheck', () => {
describe('if the grid is not sorted or paginated', () => {
const pagination = undefined;
const sortedRowMap: number[] = [];

it('returns the passed rowIndex as-is', () => {
const {
return: { getVisibleRowIndex },
} = testCustomHook(() => useVisibleRowIndex(pagination, sortedRowMap));
return: { findVisibleRowIndex },
} = testCustomHook(() => useSortPageCheck(pagination, sortedRowMap));

expect(getVisibleRowIndex(5)).toEqual(5);
expect(findVisibleRowIndex(5)).toEqual(5);
});
});

Expand All @@ -59,14 +59,14 @@ describe('useVisibleRowIndex', () => {

it('returns the visibleRowIndex of the passed rowIndex (which is the index of the sortedRowMap)', () => {
const {
return: { getVisibleRowIndex },
} = testCustomHook(() => useVisibleRowIndex(pagination, sortedRowMap));

expect(getVisibleRowIndex(0)).toEqual(4);
expect(getVisibleRowIndex(1)).toEqual(2);
expect(getVisibleRowIndex(2)).toEqual(3);
expect(getVisibleRowIndex(3)).toEqual(0);
expect(getVisibleRowIndex(4)).toEqual(1);
return: { findVisibleRowIndex },
} = testCustomHook(() => useSortPageCheck(pagination, sortedRowMap));

expect(findVisibleRowIndex(0)).toEqual(4);
expect(findVisibleRowIndex(1)).toEqual(2);
expect(findVisibleRowIndex(2)).toEqual(3);
expect(findVisibleRowIndex(3)).toEqual(0);
expect(findVisibleRowIndex(4)).toEqual(1);
});
});

Expand All @@ -83,22 +83,22 @@ describe('useVisibleRowIndex', () => {

it('calculates what page the row should be on, paginates to that page, and returns the index of the row on that page', () => {
const {
return: { getVisibleRowIndex },
} = testCustomHook(() => useVisibleRowIndex(pagination, sortedRowMap));
return: { findVisibleRowIndex },
} = testCustomHook(() => useSortPageCheck(pagination, sortedRowMap));

expect(getVisibleRowIndex(20)).toEqual(0); // First item on 2nd page
expect(findVisibleRowIndex(20)).toEqual(0); // First item on 2nd page
expect(pagination.onChangePage).toHaveBeenLastCalledWith(1);

expect(getVisibleRowIndex(75)).toEqual(15); // 16th item on 4th page
expect(findVisibleRowIndex(75)).toEqual(15); // 16th item on 4th page
expect(pagination.onChangePage).toHaveBeenLastCalledWith(3);
});

it('does not paginate if the user is already on the correct page', () => {
const {
return: { getVisibleRowIndex },
} = testCustomHook(() => useVisibleRowIndex(pagination, sortedRowMap));
return: { findVisibleRowIndex },
} = testCustomHook(() => useSortPageCheck(pagination, sortedRowMap));

expect(getVisibleRowIndex(5)).toEqual(5);
expect(findVisibleRowIndex(5)).toEqual(5);
expect(pagination.onChangePage).not.toHaveBeenCalled();
});
});
Expand Down
16 changes: 8 additions & 8 deletions src/components/datagrid/utils/ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const useImperativeGridRef = ({
}: Dependencies) => {
// Cell location helpers
const { checkCellExists } = useCellLocationCheck(rowCount, visibleColCount);
const { getVisibleRowIndex } = useVisibleRowIndex(pagination, sortedRowMap);
const { findVisibleRowIndex } = useSortPageCheck(pagination, sortedRowMap);

// Focus APIs
const { setFocusedCell: _setFocusedCell } = focusContext; // eslint complains about the dependency array otherwise
Expand All @@ -50,10 +50,10 @@ export const useImperativeGridRef = ({
const setFocusedCell = useCallback(
({ rowIndex, colIndex }) => {
checkCellExists({ rowIndex, colIndex });
const visibleRowIndex = getVisibleRowIndex(rowIndex);
const visibleRowIndex = findVisibleRowIndex(rowIndex);
_setFocusedCell([colIndex, visibleRowIndex]); // Transmog args from obj to array
},
[_setFocusedCell, checkCellExists, getVisibleRowIndex]
[_setFocusedCell, checkCellExists, findVisibleRowIndex]
);

// Popover APIs
Expand All @@ -69,10 +69,10 @@ export const useImperativeGridRef = ({
const openCellPopover = useCallback(
({ rowIndex, colIndex }) => {
checkCellExists({ rowIndex, colIndex });
const visibleRowIndex = getVisibleRowIndex(rowIndex);
const visibleRowIndex = findVisibleRowIndex(rowIndex);
_openCellPopover({ rowIndex: visibleRowIndex, colIndex });
},
[_openCellPopover, checkCellExists, getVisibleRowIndex]
[_openCellPopover, checkCellExists, findVisibleRowIndex]
);

// Set the ref APIs
Expand Down Expand Up @@ -123,11 +123,11 @@ export const useCellLocationCheck = (rowCount: number, colCount: number) => {
* the row is not on the current page, the grid should automatically handle
* paginating to that row.
*/
export const useVisibleRowIndex = (
export const useSortPageCheck = (
pagination: EuiDataGridProps['pagination'],
sortedRowMap: DataGridSortingContextShape['sortedRowMap']
) => {
const getVisibleRowIndex = useCallback(
const findVisibleRowIndex = useCallback(
(rowIndex: number): number => {
// Account for sorting
const visibleRowIndex = sortedRowMap.length
Expand All @@ -150,5 +150,5 @@ export const useVisibleRowIndex = (
[pagination, sortedRowMap]
);

return { getVisibleRowIndex };
return { findVisibleRowIndex };
};

0 comments on commit 55a4ba6

Please sign in to comment.