From 472e5d210234b359b3b0c2e176fce0983701b76a Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 21 Nov 2019 14:51:47 -0700 Subject: [PATCH 1/4] Using hooks & state to subscribe to re-renders --- src/components/datagrid/data_grid.tsx | 54 ++++++++++++++++++++------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/src/components/datagrid/data_grid.tsx b/src/components/datagrid/data_grid.tsx index 832f254812c..7f1b2fbc5cc 100644 --- a/src/components/datagrid/data_grid.tsx +++ b/src/components/datagrid/data_grid.tsx @@ -290,7 +290,7 @@ function createKeyDownHandler( focusedCell: [number, number], headerIsInteractive: boolean, setFocusedCell: (focusedCell: [number, number]) => void, - updateFocus: (focusedCell: [number, number]) => void + updateFocus: Function ) { return (event: KeyboardEvent) => { const colCount = visibleColumns.length - 1; @@ -340,7 +340,7 @@ function createKeyDownHandler( ? focusedCell[1] : newPageRowCount - 1; setFocusedCell([focusedCell[0], rowIndex]); - updateFocus([focusedCell[0], rowIndex]); + updateFocus(); } } } else if (keyCode === keyCodes.PAGE_UP) { @@ -349,7 +349,7 @@ function createKeyDownHandler( const pageIndex = props.pagination.pageIndex; if (pageIndex > 0) { props.pagination.onChangePage(pageIndex - 1); - updateFocus(focusedCell); + updateFocus(); } } } else if (keyCode === (ctrlKey && keyCodes.END)) { @@ -368,6 +368,20 @@ function createKeyDownHandler( }; } +function useAfterRender(): [unknown, Function] { + const [isSubscribed, setIsSubscribed] = useState(false); + const [subscription, setSubscription] = useState(0); + + useEffect(() => { + if (isSubscribed) { + setIsSubscribed(false); + setSubscription(subscription => ++subscription); + } + }, [isSubscribed, setSubscription]); + + return [subscription, () => setIsSubscribed(true)]; +} + export const EuiDataGrid: FunctionComponent = props => { const [isFullScreen, setIsFullScreen] = useState(false); const [hasRoomForGridControls, setHasRoomForGridControls] = useState(true); @@ -612,16 +626,20 @@ export const EuiDataGrid: FunctionComponent = props => { ); - const [cellsUpdateFocus] = useState>(new Map()); + const [cellsUpdateFocus, setCellsUpdateFocus] = useState< + Map + >(new Map()); - const updateFocus = (focusedCell: [number, number]) => { - const key = `${focusedCell[0]}-${focusedCell[1]}`; - if (cellsUpdateFocus.has(key)) { - requestAnimationFrame(() => { + const [renderSubscription, subscribeToRender] = useAfterRender(); + useEffect(() => { + if (focusedCell) { + const key = `${focusedCell[0]}-${focusedCell[1]}`; + + if (cellsUpdateFocus.has(key)) { cellsUpdateFocus.get(key)!(); - }); + } } - }; + }, [renderSubscription]); const datagridContext = { onFocusUpdate: (cell: [number, number], updateFocus: Function) => { @@ -631,10 +649,20 @@ export const EuiDataGrid: FunctionComponent = props => { // this intentionally and purposefully mutates the existing `cellsUpdateFocus` object as the // value/state of `cellsUpdateFocus` must be up-to-date when `updateFocus`'s requestAnimationFrame fires // there is likely a better pattern to use, but this is fine for now as the scope is known & limited - cellsUpdateFocus.set(key, updateFocus); + // cellsUpdateFocus.set(key, updateFocus); + + setCellsUpdateFocus(cellsUpdateFocus => { + const nextCellsUpdateFocus = new Map(cellsUpdateFocus); + nextCellsUpdateFocus.set(key, updateFocus); + return nextCellsUpdateFocus; + }); return () => { - cellsUpdateFocus.delete(key); + setCellsUpdateFocus(cellsUpdateFocus => { + const nextCellsUpdateFocus = new Map(cellsUpdateFocus); + nextCellsUpdateFocus.delete(key); + return nextCellsUpdateFocus; + }); }; } }, @@ -669,7 +697,7 @@ export const EuiDataGrid: FunctionComponent = props => { realizedFocusedCell, headerIsInteractive, setFocusedCell, - updateFocus + subscribeToRender )} className="euiDataGrid__verticalScroll" ref={resizeRef} From 453a8ba5ea0c3150f2c6fd8d844b9122aa192d59 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 22 Nov 2019 07:58:41 -0700 Subject: [PATCH 2/4] modify hook to wait for a double render --- src/components/datagrid/data_grid.tsx | 29 ++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/components/datagrid/data_grid.tsx b/src/components/datagrid/data_grid.tsx index 7f1b2fbc5cc..88720ad4e9d 100644 --- a/src/components/datagrid/data_grid.tsx +++ b/src/components/datagrid/data_grid.tsx @@ -368,18 +368,30 @@ function createKeyDownHandler( }; } -function useAfterRender(): [unknown, Function] { +function useAfterRender(fn: Function): Function { const [isSubscribed, setIsSubscribed] = useState(false); - const [subscription, setSubscription] = useState(0); + const [needsExecution, setNeedsExecution] = useState(false); + // first useEffect waits for the parent & children to render & flush to dom useEffect(() => { if (isSubscribed) { setIsSubscribed(false); - setSubscription(subscription => ++subscription); + setNeedsExecution(true); } - }, [isSubscribed, setSubscription]); + }, [isSubscribed, setIsSubscribed, setNeedsExecution]); - return [subscription, () => setIsSubscribed(true)]; + // second useEffect allows for a new `fn` to have been created + // with any state updates before being called + useEffect(() => { + if (needsExecution) { + setNeedsExecution(false); + fn(); + } + }, [needsExecution, setNeedsExecution, fn]); + + return () => { + setIsSubscribed(true); + }; } export const EuiDataGrid: FunctionComponent = props => { @@ -630,8 +642,7 @@ export const EuiDataGrid: FunctionComponent = props => { Map >(new Map()); - const [renderSubscription, subscribeToRender] = useAfterRender(); - useEffect(() => { + const focusAfterRender = useAfterRender(() => { if (focusedCell) { const key = `${focusedCell[0]}-${focusedCell[1]}`; @@ -639,7 +650,7 @@ export const EuiDataGrid: FunctionComponent = props => { cellsUpdateFocus.get(key)!(); } } - }, [renderSubscription]); + }); const datagridContext = { onFocusUpdate: (cell: [number, number], updateFocus: Function) => { @@ -697,7 +708,7 @@ export const EuiDataGrid: FunctionComponent = props => { realizedFocusedCell, headerIsInteractive, setFocusedCell, - subscribeToRender + focusAfterRender )} className="euiDataGrid__verticalScroll" ref={resizeRef} From 2959682f16af9837b4dca4e3ee40dc1d47eae547 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 6 Dec 2019 10:44:42 -0700 Subject: [PATCH 3/4] removed old code&comment --- src/components/datagrid/data_grid.tsx | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/components/datagrid/data_grid.tsx b/src/components/datagrid/data_grid.tsx index 88720ad4e9d..92cc103edc7 100644 --- a/src/components/datagrid/data_grid.tsx +++ b/src/components/datagrid/data_grid.tsx @@ -657,11 +657,6 @@ export const EuiDataGrid: FunctionComponent = props => { if (pagination) { const key = `${cell[0]}-${cell[1]}`; - // this intentionally and purposefully mutates the existing `cellsUpdateFocus` object as the - // value/state of `cellsUpdateFocus` must be up-to-date when `updateFocus`'s requestAnimationFrame fires - // there is likely a better pattern to use, but this is fine for now as the scope is known & limited - // cellsUpdateFocus.set(key, updateFocus); - setCellsUpdateFocus(cellsUpdateFocus => { const nextCellsUpdateFocus = new Map(cellsUpdateFocus); nextCellsUpdateFocus.set(key, updateFocus); From bfd0cb9249a4966c3312129337de7812f1c03d16 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 6 Dec 2019 11:00:03 -0700 Subject: [PATCH 4/4] changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96716d19900..fafc44d507f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## [`master`](https://github.com/elastic/eui/tree/master) -No public interface changes since `17.0.0`. +**Bug fixes** + +- Fixed UX/focus bug in `EuiDataGrid` when using keyboard shortcuts to paginate ([#2602](https://github.com/elastic/eui/pull/2602)) ## [`17.0.0`](https://github.com/elastic/eui/tree/v17.0.0)