From 6ace0971f334c223a495bd3eab3fa37d21f685b6 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Mar 2022 13:33:07 -0800 Subject: [PATCH 1/9] [REVERT] Test rowHeightsOptions props --- src-docs/src/views/datagrid/datagrid.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src-docs/src/views/datagrid/datagrid.js b/src-docs/src/views/datagrid/datagrid.js index 9dfdc24fc8c..faab5d60f73 100644 --- a/src-docs/src/views/datagrid/datagrid.js +++ b/src-docs/src/views/datagrid/datagrid.js @@ -12,6 +12,7 @@ import { Link } from 'react-router-dom'; import { fake } from 'faker'; import { + EuiFieldNumber, EuiButton, EuiButtonEmpty, EuiButtonIcon, @@ -375,6 +376,10 @@ const trailingControlColumns = [ ]; export default () => { + // row heights testing + const [rowHeight, setRowHeight] = useState(36); + const [lineCount, setLineCount] = useState(0); + // Pagination const [pagination, setPagination] = useState({ pageIndex: 0, pageSize: 10 }); const onChangeItemsPerPage = useCallback( @@ -412,6 +417,20 @@ export default () => { return ( + setLineCount(Number(e.currentTarget.value))} + /> + setRowHeight(Number(e.currentTarget.value))} + /> { }} onColumnResize={onColumnResize.current} ref={gridRef} + rowHeightsOptions={ + lineCount + ? { defaultHeight: { lineCount } } + : { defaultHeight: rowHeight } + } + height={600} /> ); From 2097334333bcbdd57f9b6cd499748af32f55a94e Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Mar 2022 13:44:28 -0800 Subject: [PATCH 2/9] Force a rerender whenever rowHeightsOptions (that affect row heights) change --- src/components/datagrid/body/data_grid_body.tsx | 1 + src/components/datagrid/utils/row_heights.ts | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/components/datagrid/body/data_grid_body.tsx b/src/components/datagrid/body/data_grid_body.tsx index 64ffe016ca2..e84e497240a 100644 --- a/src/components/datagrid/body/data_grid_body.tsx +++ b/src/components/datagrid/body/data_grid_body.tsx @@ -391,6 +391,7 @@ export const EuiDataGridBody: FunctionComponent = ( gridRef: gridRef.current, gridStyles, columns, + rowHeightsOptions, }); const { defaultRowHeight, setRowHeight, getRowHeight } = useDefaultRowHeight({ diff --git a/src/components/datagrid/utils/row_heights.ts b/src/components/datagrid/utils/row_heights.ts index 4dc44269a3e..d07035cd499 100644 --- a/src/components/datagrid/utils/row_heights.ts +++ b/src/components/datagrid/utils/row_heights.ts @@ -16,6 +16,7 @@ import { } from 'react'; import type { VariableSizeGrid as Grid } from 'react-window'; import { isObject, isNumber } from '../../../services/predicate'; +import { useForceRender } from '../../../services'; import { EuiDataGridStyleCellPaddings, EuiDataGridStyle, @@ -253,10 +254,12 @@ export const useRowHeightUtils = ({ gridRef, gridStyles, columns, + rowHeightsOptions, }: { gridRef: Grid | null; gridStyles: EuiDataGridStyle; columns: EuiDataGridColumn[]; + rowHeightsOptions?: EuiDataGridRowHeightsOptions; }) => { const rowHeightUtils = useMemo(() => new RowHeightUtils(), []); @@ -265,6 +268,19 @@ export const useRowHeightUtils = ({ if (gridRef) rowHeightUtils.setGrid(gridRef); }, [gridRef, rowHeightUtils]); + // Forces a rerender whenever the row heights change, as this can cause the + // grid to change height/have scrollbars. Without this, grid rerendering is stale + useEffect(() => { + requestAnimationFrame(forceRender); + }, [ + // Effects that should cause rerendering + rowHeightsOptions?.defaultHeight, + rowHeightsOptions?.rowHeights, + // Dependencies + rowHeightUtils, + forceRender, + ]); + // Re-cache styles whenever grid density changes useEffect(() => { rowHeightUtils.cacheStyles({ From 085d16071af19940443e2aa93f4b746ebd980f0d Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Mar 2022 13:45:52 -0800 Subject: [PATCH 3/9] Misc cleanup - move rowHeightUtils.setRerenderGridBody to same location as rowHeightUtils.setGrid - move inline comment to where fn is being invoked - update useRowHeightUtils hook comment --- src/components/datagrid/utils/grid_height_width.ts | 7 +------ src/components/datagrid/utils/row_heights.ts | 13 +++++++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/components/datagrid/utils/grid_height_width.ts b/src/components/datagrid/utils/grid_height_width.ts index 0b901720515..db87f45e5a7 100644 --- a/src/components/datagrid/utils/grid_height_width.ts +++ b/src/components/datagrid/utils/grid_height_width.ts @@ -99,12 +99,6 @@ export const useUnconstrainedHeight = ({ }) => { const { getCorrectRowIndex } = useContext(DataGridSortingContext); - // when a row height is updated, force a re-render of the grid body to update the unconstrained height - const forceRender = useForceRender(); - useEffect(() => { - rowHeightUtils.setRerenderGridBody(forceRender); - }, [rowHeightUtils, forceRender]); - let knownHeight = 0; // tracks the pixel height of rows we know the size of let knownRowCount = 0; // how many rows we know the size of for (let i = startRow; i < endRow; i++) { @@ -137,6 +131,7 @@ export const useUnconstrainedHeight = ({ innerGridRef.current, 'width' ); + const forceRender = useForceRender(); useUpdateEffect(forceRender, [innerWidth]); const unconstrainedHeight = diff --git a/src/components/datagrid/utils/row_heights.ts b/src/components/datagrid/utils/row_heights.ts index d07035cd499..23f2b5b1afa 100644 --- a/src/components/datagrid/utils/row_heights.ts +++ b/src/components/datagrid/utils/row_heights.ts @@ -202,6 +202,9 @@ export class RowHeightUtils { rowHeights.set(colId, adaptedHeight); this.heightsCache.set(rowIndex, rowHeights); this.resetRow(visibleRowIndex); + + // When an auto row height is updated, force a re-render + // of the grid body to update the unconstrained height this.rerenderGridBody(); } @@ -247,8 +250,8 @@ export class RowHeightUtils { } /** - * Hook for instantiating RowHeightUtils, and also updating - * internal vars from outside props via useEffects + * Hook for instantiating RowHeightUtils, setting internal class vars, + * and setting up various row-height-related side effects */ export const useRowHeightUtils = ({ gridRef, @@ -263,10 +266,12 @@ export const useRowHeightUtils = ({ }) => { const rowHeightUtils = useMemo(() => new RowHeightUtils(), []); - // Update rowHeightUtils with grid ref + // Update rowHeightUtils with internal vars from outside dependencies + const forceRender = useForceRender(); useEffect(() => { if (gridRef) rowHeightUtils.setGrid(gridRef); - }, [gridRef, rowHeightUtils]); + rowHeightUtils.setRerenderGridBody(forceRender); + }, [gridRef, forceRender, rowHeightUtils]); // Forces a rerender whenever the row heights change, as this can cause the // grid to change height/have scrollbars. Without this, grid rerendering is stale From 937b6e5109fe33108def82b421ac91edcdd09a90 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Mar 2022 13:55:13 -0800 Subject: [PATCH 4/9] Work around failing data_grid.test.tsx tests - not totally sure why this requestAnimationFrame is causing issues but not others --- src/components/datagrid/data_grid.test.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/components/datagrid/data_grid.test.tsx b/src/components/datagrid/data_grid.test.tsx index 2f03c240189..4d5bd581306 100644 --- a/src/components/datagrid/data_grid.test.tsx +++ b/src/components/datagrid/data_grid.test.tsx @@ -452,6 +452,11 @@ function moveColumnToIndex( } describe('EuiDataGrid', () => { + // Mock requestAnimationFrame to run immediately + jest + .spyOn(window, 'requestAnimationFrame') + .mockImplementation((cb: any) => cb()); + describe('rendering', () => { const getBoundingClientRect = window.Element.prototype.getBoundingClientRect; From 751c70ff544d11026a8598dd5770e4adbbce355d Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 10 Mar 2022 14:19:14 -0800 Subject: [PATCH 5/9] Add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9c695e9877..59ce35cb0f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## [`main`](https://github.com/elastic/eui/tree/main) +**Bug fixes** + +- Fixed EuiDataGrid not rerendering correctly on row heights change ([#5712](https://github.com/elastic/eui/pull/5712)) + **Breaking changes** - Removed Legacy theme including compiled CSS ([#5688](https://github.com/elastic/eui/pull/5688)) From a7ebb95ea89e8cc94698b74661da4abd92205df9 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 17 Mar 2022 13:05:10 -0700 Subject: [PATCH 6/9] changelog --- CHANGELOG.md | 4 ---- upcoming_changelogs/5712.md | 3 +++ 2 files changed, 3 insertions(+), 4 deletions(-) create mode 100644 upcoming_changelogs/5712.md diff --git a/CHANGELOG.md b/CHANGELOG.md index cb1ce78ee12..48313c4b28b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,10 +5,6 @@ - Added `compressed` prop to `EuiFilterGroup` and reduced the size of the `EuiFilterButton` notification badge ([#5717](https://github.com/elastic/eui/pull/5717)) - Increased contrast of `EuiSelectableTemplateSitewide` input text when in dark header ([#5724](https://github.com/elastic/eui/pull/5724)) -**Bug fixes** - -- Fixed EuiDataGrid not rerendering correctly on row heights change ([#5712](https://github.com/elastic/eui/pull/5712)) - **Breaking changes** - Removed Legacy theme including compiled CSS ([#5688](https://github.com/elastic/eui/pull/5688)) diff --git a/upcoming_changelogs/5712.md b/upcoming_changelogs/5712.md new file mode 100644 index 00000000000..e9bb7cd7fed --- /dev/null +++ b/upcoming_changelogs/5712.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- Fixed EuiDataGrid not rerendering correctly on row heights change From ccd938300a4f8fb1bb5797c96992be6a2efbcbf4 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 18 Mar 2022 12:59:02 -0700 Subject: [PATCH 7/9] Add useRowHeightUtils unit tests --- .../datagrid/utils/row_heights.test.ts | 99 ++++++++++++++++++- 1 file changed, 98 insertions(+), 1 deletion(-) diff --git a/src/components/datagrid/utils/row_heights.test.ts b/src/components/datagrid/utils/row_heights.test.ts index 7661a662516..36cd3414fc9 100644 --- a/src/components/datagrid/utils/row_heights.test.ts +++ b/src/components/datagrid/utils/row_heights.test.ts @@ -6,7 +6,15 @@ * Side Public License, v 1. */ -import { RowHeightUtils, cellPaddingsMap } from './row_heights'; +import { act } from 'react-dom/test-utils'; +import { testCustomHook } from '../../../test/test_custom_hook.test_helper'; +import { startingStyles } from '../controls'; + +import { + RowHeightUtils, + cellPaddingsMap, + useRowHeightUtils, +} from './row_heights'; describe('RowHeightUtils', () => { const rowHeightUtils = new RowHeightUtils(); @@ -371,3 +379,92 @@ describe('RowHeightUtils', () => { }); }); }); + +describe('useRowHeightUtils', () => { + const mockArgs = { + gridRef: null, + gridStyles: startingStyles, + columns: [{ id: 'A' }, { id: 'B' }], + rowHeightOptions: undefined, + }; + + it('instantiates and returns an instance of RowHeightUtils', () => { + const { return: rowHeightUtils } = testCustomHook(() => + useRowHeightUtils(mockArgs) + ); + expect(rowHeightUtils).toBeInstanceOf(RowHeightUtils); + }); + + it('populates internal RowHeightUtils vars from outside dependencies', () => { + const args = { ...mockArgs, gridRef: {} as any }; + const { return: rowHeightUtils } = testCustomHook(() => + useRowHeightUtils(args) + ); + // @ts-ignore - intentionally inspecting private var for test + expect(rowHeightUtils.grid).toEqual(args.gridRef); + // @ts-ignore - intentionally inspecting private var for test + expect(rowHeightUtils.rerenderGridBody).toBeInstanceOf(Function); + }); + + it('forces a rerender every time rowHeightsOptions changes', () => { + const requestAnimationFrameSpy = jest + .spyOn(window, 'requestAnimationFrame') + .mockImplementation((cb: any) => cb()); + + const { updateHookArgs } = testCustomHook(useRowHeightUtils, mockArgs); + expect(requestAnimationFrameSpy).toHaveBeenCalledTimes(1); + + updateHookArgs({ rowHeightsOptions: { defaultHeight: 300 } }); + expect(requestAnimationFrameSpy).toHaveBeenCalledTimes(2); + + updateHookArgs({ + rowHeightsOptions: { defaultHeight: 300, rowHeights: { 0: 200 } }, + }); + expect(requestAnimationFrameSpy).toHaveBeenCalledTimes(3); + }); + + it('updates internal cached styles whenever gridStyle.cellPadding changes', () => { + const { return: rowHeightUtils, updateHookArgs } = testCustomHook( + useRowHeightUtils, + mockArgs + ); + + updateHookArgs({ gridStyles: { ...startingStyles, cellPadding: 's' } }); + // @ts-ignore - intentionally inspecting private var for test + expect(rowHeightUtils.styles).toEqual({ + paddingTop: 4, + paddingBottom: 4, + }); + }); + + it('prunes columns from the row heights cache if a column is hidden', () => { + const { return: rowHeightUtils, updateHookArgs } = testCustomHook< + RowHeightUtils + >(useRowHeightUtils, mockArgs); + + act(() => { + rowHeightUtils.setRowHeight(0, 'A', 30, 0); + rowHeightUtils.setRowHeight(0, 'B', 50, 0); + }); + // @ts-ignore - intentionally inspecting private var for test + expect(rowHeightUtils.heightsCache).toMatchInlineSnapshot(` + Map { + 0 => Map { + "A" => 42, + "B" => 62, + }, + } + `); + + updateHookArgs({ columns: [{ id: 'A' }] }); // Hiding column B + + // @ts-ignore - intentionally inspecting private var for test + expect(rowHeightUtils.heightsCache).toMatchInlineSnapshot(` + Map { + 0 => Map { + "A" => 42, + }, + } + `); + }); +}); From 38d201468f2c28b5ff68489ebc95ea1ad5b6d732 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 18 Mar 2022 13:15:39 -0700 Subject: [PATCH 8/9] [misc fix] Handle consumers specifically setting `gridStyle.cellPadding` to undefined - rather than just omitting it - see https://github.com/elastic/eui/pull/5713#discussion_r827327926 --- src/components/datagrid/utils/row_heights.test.ts | 10 ++++++++++ src/components/datagrid/utils/row_heights.ts | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/components/datagrid/utils/row_heights.test.ts b/src/components/datagrid/utils/row_heights.test.ts index 36cd3414fc9..53328d8cfc3 100644 --- a/src/components/datagrid/utils/row_heights.test.ts +++ b/src/components/datagrid/utils/row_heights.test.ts @@ -155,6 +155,16 @@ describe('RowHeightUtils', () => { }); }); }); + + it('falls back to m-sized cellPadding if gridStyle.cellPadding is undefined', () => { + rowHeightUtils.cacheStyles({ cellPadding: undefined }); + + // @ts-ignore this var is private, but we're inspecting it for the sake of the unit test + expect(rowHeightUtils.styles).toEqual({ + paddingTop: 6, + paddingBottom: 6, + }); + }); }); describe('getStylesForCell (returns inline CSS styles based on height config)', () => { diff --git a/src/components/datagrid/utils/row_heights.ts b/src/components/datagrid/utils/row_heights.ts index 23f2b5b1afa..6d313fcf4a4 100644 --- a/src/components/datagrid/utils/row_heights.ts +++ b/src/components/datagrid/utils/row_heights.ts @@ -97,8 +97,8 @@ export class RowHeightUtils { cacheStyles(gridStyles: EuiDataGridStyle) { this.styles = { - paddingTop: cellPaddingsMap[gridStyles.cellPadding!], - paddingBottom: cellPaddingsMap[gridStyles.cellPadding!], + paddingTop: cellPaddingsMap[gridStyles.cellPadding || 'm'], + paddingBottom: cellPaddingsMap[gridStyles.cellPadding || 'm'], }; } From f830ca7ef8219fadf0c72ebb2f7211dfaf7eb349 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Fri, 18 Mar 2022 13:17:31 -0700 Subject: [PATCH 9/9] Revert demo example --- .../src/views/datagrid/basics/datagrid.js | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/src-docs/src/views/datagrid/basics/datagrid.js b/src-docs/src/views/datagrid/basics/datagrid.js index bcb9e97fe5d..49de063d996 100644 --- a/src-docs/src/views/datagrid/basics/datagrid.js +++ b/src-docs/src/views/datagrid/basics/datagrid.js @@ -12,7 +12,6 @@ import { Link } from 'react-router-dom'; import { fake } from 'faker'; import { - EuiFieldNumber, EuiButton, EuiButtonEmpty, EuiButtonIcon, @@ -376,10 +375,6 @@ const trailingControlColumns = [ ]; export default () => { - // row heights testing - const [rowHeight, setRowHeight] = useState(36); - const [lineCount, setLineCount] = useState(0); - // Pagination const [pagination, setPagination] = useState({ pageIndex: 0, pageSize: 10 }); const onChangeItemsPerPage = useCallback( @@ -417,20 +412,6 @@ export default () => { return ( - setLineCount(Number(e.currentTarget.value))} - /> - setRowHeight(Number(e.currentTarget.value))} - /> { }} onColumnResize={onColumnResize.current} ref={gridRef} - rowHeightsOptions={ - lineCount - ? { defaultHeight: { lineCount } } - : { defaultHeight: rowHeight } - } - height={600} /> );