From 9a0a65ba456347480433e1752737a16219284a91 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 11 Nov 2021 14:41:26 -0800 Subject: [PATCH 1/9] [REVERT ME] test lineCount override fix --- src-docs/src/views/datagrid/row_height_fixed.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src-docs/src/views/datagrid/row_height_fixed.tsx b/src-docs/src/views/datagrid/row_height_fixed.tsx index 91df500a1ca..ab5635dbfae 100644 --- a/src-docs/src/views/datagrid/row_height_fixed.tsx +++ b/src-docs/src/views/datagrid/row_height_fixed.tsx @@ -184,13 +184,11 @@ export default () => { // matches the snippet example const rowHeightsOptions = useMemo( () => ({ - defaultHeight: 140, + defaultHeight: { lineCount: 3 }, rowHeights: { - 1: { - lineCount: 5, - }, - 4: 200, - 5: 80, + 1: { lineCount: 5 }, + 4: { lineCount: 1 }, + 5: { lineCount: 10 }, }, }), [] From 8a2fce698b7625eab1a232faf360aa6e5a43a781 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 11 Nov 2021 13:11:00 -0800 Subject: [PATCH 2/9] Simplify auto row height docs example - Remove line overrides from auto example (causes confusion during testing, isn't necessary) --- .../views/datagrid/datagrid_height_options_example.js | 10 +--------- src-docs/src/views/datagrid/row_height_auto.tsx | 6 ------ 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src-docs/src/views/datagrid/datagrid_height_options_example.js b/src-docs/src/views/datagrid/datagrid_height_options_example.js index 596d9d2afd9..d0d9e0158d5 100644 --- a/src-docs/src/views/datagrid/datagrid_height_options_example.js +++ b/src-docs/src/views/datagrid/datagrid_height_options_example.js @@ -61,9 +61,7 @@ const rowHeightsSnippet = `rowHeightsOptions = { const rowHeightsFullSnippet = `const rowHeightsOptions = useMemo( () => ({ defaultHeight: 140, - }), - [] -); +}; { const rowHeightsOptions = useMemo( () => ({ defaultHeight: 'auto' as const, - rowHeights: { - 1: { - lineCount: 5, - }, - 4: 140, - }, }), [] ); From f132c6c4e3745d95ce7114bfdc608d22ca76156d Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 11 Nov 2021 14:32:22 -0800 Subject: [PATCH 3/9] Fix rowHeights with row-specific lineCount heights to use the height cache instead of setting the minRowHeight - the lineCount overrides are causing incorrect minRowHeight calculations for other rows, so they need their own specific map, and the auto one is right there :shrug: --- .../datagrid/__mocks__/row_height_utils.ts | 1 + .../data_grid_cell.test.tsx.snap | 2 + .../datagrid/body/data_grid_body.tsx | 3 +- .../datagrid/body/data_grid_cell.test.tsx | 37 +++++++--- .../datagrid/body/data_grid_cell.tsx | 12 +++- .../datagrid/row_height_utils.test.ts | 70 +++++++++++++++++-- src/components/datagrid/row_height_utils.ts | 42 +++++++---- 7 files changed, 134 insertions(+), 33 deletions(-) diff --git a/src/components/datagrid/__mocks__/row_height_utils.ts b/src/components/datagrid/__mocks__/row_height_utils.ts index c50c7eff0f5..dd8f6ffedef 100644 --- a/src/components/datagrid/__mocks__/row_height_utils.ts +++ b/src/components/datagrid/__mocks__/row_height_utils.ts @@ -26,6 +26,7 @@ export const mockRowHeightUtils = ({ getCalculatedHeight: jest.fn(actual.getCalculatedHeight), getLineCount: jest.fn(actual.getLineCount), calculateHeightForLineCount: jest.fn(() => 50), + isRowHeightOverride: jest.fn(actual.isRowHeightOverride), } as unknown) as ActualRowHeightUtils; export const RowHeightUtils = jest.fn(() => mockRowHeightUtils); diff --git a/src/components/datagrid/body/__snapshots__/data_grid_cell.test.tsx.snap b/src/components/datagrid/body/__snapshots__/data_grid_cell.test.tsx.snap index d8d42027616..a2fce7944c6 100644 --- a/src/components/datagrid/body/__snapshots__/data_grid_cell.test.tsx.snap +++ b/src/components/datagrid/body/__snapshots__/data_grid_cell.test.tsx.snap @@ -31,6 +31,7 @@ exports[`EuiDataGridCell renders 1`] = ` }, "getStylesForCell": [MockFunction], "isAutoHeight": [MockFunction], + "isRowHeightOverride": [MockFunction], "pruneHiddenColumnHeights": [MockFunction], "setGrid": [MockFunction], "setRowHeight": [MockFunction], @@ -93,6 +94,7 @@ exports[`EuiDataGridCell renders 1`] = ` }, "getStylesForCell": [MockFunction], "isAutoHeight": [MockFunction], + "isRowHeightOverride": [MockFunction], "pruneHiddenColumnHeights": [MockFunction], "setGrid": [MockFunction], "setRowHeight": [MockFunction], diff --git a/src/components/datagrid/body/data_grid_body.tsx b/src/components/datagrid/body/data_grid_body.tsx index 47e54c10295..40dd820669f 100644 --- a/src/components/datagrid/body/data_grid_body.tsx +++ b/src/components/datagrid/body/data_grid_body.tsx @@ -532,7 +532,8 @@ export const EuiDataGridBody: FunctionComponent = ( height = rowHeightUtils.getCalculatedHeight( rowHeightOption, minRowHeight, - correctRowIndex + correctRowIndex, + rowHeightUtils.isRowHeightOverride(correctRowIndex, rowHeightsOptions) ); } diff --git a/src/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx index ee8aa2fd257..5de27836e6d 100644 --- a/src/components/datagrid/body/data_grid_cell.test.tsx +++ b/src/components/datagrid/body/data_grid_cell.test.tsx @@ -41,6 +41,8 @@ describe('EuiDataGridCell', () => { }); }; + beforeEach(() => jest.clearAllMocks()); + it('renders', () => { const component = mountEuiDataGridCellWithContext(); expect(component).toMatchSnapshot(); @@ -172,9 +174,6 @@ describe('EuiDataGridCell', () => { // TODO: Test ResizeObserver logic in Cypress alongside Jest describe('row height logic & resize observers', () => { describe('recalculateAutoHeight', () => { - beforeEach(() => { - (mockRowHeightUtils.setRowHeight as jest.Mock).mockClear(); - }); afterEach(() => { (mockRowHeightUtils.isAutoHeight as jest.Mock).mockRestore(); }); @@ -207,19 +206,37 @@ describe('EuiDataGridCell', () => { describe('recalculateLineCountHeight', () => { const setRowHeight = jest.fn(); - beforeEach(() => setRowHeight.mockClear()); const callMethod = (component: ReactWrapper) => (component.instance() as any).recalculateLineCountHeight(); - it('observes the first cell for size changes and calls this.props.setRowHeight on change', () => { - const component = mountEuiDataGridCellWithContext({ - rowHeightsOptions: { defaultHeight: { lineCount: 3 } }, - setRowHeight, + describe('default height', () => { + it('observes the first cell for size changes and calls this.props.setRowHeight on change', () => { + const component = mountEuiDataGridCellWithContext({ + rowHeightsOptions: { defaultHeight: { lineCount: 3 } }, + setRowHeight, + }); + + callMethod(component); + expect(setRowHeight).toHaveBeenCalled(); }); + }); - callMethod(component); - expect(setRowHeight).toHaveBeenCalled(); + describe('row height overrides', () => { + it('uses the rowHeightUtils.setRowHeight cache instead of this.props.setRowHeight', () => { + const component = mountEuiDataGridCellWithContext({ + rowHeightsOptions: { + defaultHeight: { lineCount: 3 }, + rowHeights: { 10: { lineCount: 10 } }, + }, + rowIndex: 10, + setRowHeight, + }); + + callMethod(component); + expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalled(); + expect(setRowHeight).not.toHaveBeenCalled(); + }); }); it('does nothing if cell height is not set to lineCount', () => { diff --git a/src/components/datagrid/body/data_grid_cell.tsx b/src/components/datagrid/body/data_grid_cell.tsx index 581c89706e5..776da1e0d94 100644 --- a/src/components/datagrid/body/data_grid_cell.tsx +++ b/src/components/datagrid/body/data_grid_cell.tsx @@ -204,7 +204,17 @@ export class EuiDataGridCell extends Component< lineCount ); - this.props.setRowHeight(height); + if (rowHeightUtils?.isRowHeightOverride(rowIndex, rowHeightsOptions)) { + const { columnId, visibleRowIndex } = this.props; + rowHeightUtils?.setRowHeight( + rowIndex, + columnId, + height, + visibleRowIndex + ); + } else { + this.props.setRowHeight(height); + } } }; diff --git a/src/components/datagrid/row_height_utils.test.ts b/src/components/datagrid/row_height_utils.test.ts index 66a22481d02..592591f2c07 100644 --- a/src/components/datagrid/row_height_utils.test.ts +++ b/src/components/datagrid/row_height_utils.test.ts @@ -38,15 +38,29 @@ describe('RowHeightUtils', () => { }); }); - describe('getCalculatedHeight', () => { - describe('lineCounts', () => { - it('returns the min/defaultHeight set by the data_grid_body state', () => { - expect( - rowHeightUtils.getCalculatedHeight({ lineCount: 3 }, 50) - ).toEqual(50); - }); + describe('isRowHeightOverride', () => { + it('returns true if the passed rowIndex exists in rowHeights', () => { + expect( + rowHeightUtils.isRowHeightOverride(1, { + rowHeights: { 1: 'auto' }, + }) + ).toEqual(true); }); + it('returns false otherwise', () => { + expect( + rowHeightUtils.isRowHeightOverride(1, { + rowHeights: { 2: 'auto' }, + }) + ).toEqual(false); + expect(rowHeightUtils.isRowHeightOverride(1, undefined)).toEqual(false); + expect( + rowHeightUtils.isRowHeightOverride(1, { lineHeight: '2em' }) + ).toEqual(false); + }); + }); + + describe('getCalculatedHeight', () => { describe('static height numbers', () => { it('returns the height number', () => { expect(rowHeightUtils.getCalculatedHeight({ height: 100 }, 34)).toEqual( @@ -66,6 +80,34 @@ describe('RowHeightUtils', () => { }); }); + describe('lineCounts', () => { + describe('default heights', () => { + it('returns the min/defaultHeight set by the data_grid_body state', () => { + const defaultHeight = 50; + expect( + rowHeightUtils.getCalculatedHeight({ lineCount: 3 }, defaultHeight) + ).toEqual(defaultHeight); + }); + }); + + describe('row-specific overrides', () => { + it('returns the height set in the cache', () => { + const rowIndex = 5; + const rowHeightOverride = 100; + rowHeightUtils.setRowHeight(rowIndex, 'a', rowHeightOverride, 0); + + expect( + rowHeightUtils.getCalculatedHeight( + { lineCount: 10 }, + 34, + rowIndex, + true + ) + ).toEqual(rowHeightOverride); + }); + }); + }); + describe('auto height', () => { const getRowHeightSpy = jest.spyOn(rowHeightUtils, 'getRowHeight'); beforeEach(() => getRowHeightSpy.mockClear()); @@ -167,6 +209,20 @@ describe('RowHeightUtils', () => { 132 ); // 5 * 24 + 6 + 6 }); + + it('excludes padding calculations when the excludePadding flag is true', () => { + // This is primarily used for rowHeight lineCount overrides that use the height cache, + // which already has padding calculations built in + expect( + rowHeightUtils.calculateHeightForLineCount(cell, 1, true) + ).toEqual(24); // 1 * 24 + expect( + rowHeightUtils.calculateHeightForLineCount(cell, 3, true) + ).toEqual(72); // 3 * 24 + expect( + rowHeightUtils.calculateHeightForLineCount(cell, 5, true) + ).toEqual(120); // 5 * 24 + }); }); }); diff --git a/src/components/datagrid/row_height_utils.ts b/src/components/datagrid/row_height_utils.ts index 383003efe23..f3822d4aa0a 100644 --- a/src/components/datagrid/row_height_utils.ts +++ b/src/components/datagrid/row_height_utils.ts @@ -38,24 +38,35 @@ export class RowHeightUtils { ); } + isRowHeightOverride( + rowIndex: number, + rowHeightsOptions?: EuiDataGridRowHeightsOptions + ): boolean { + return rowHeightsOptions?.rowHeights?.[rowIndex] != null; + } + getCalculatedHeight( heightOption: EuiDataGridRowHeightOption, defaultHeight: number, - rowIndex?: number + rowIndex?: number, + isRowHeightOverride?: boolean ) { - if (isObject(heightOption)) { - if (heightOption.lineCount) { - return defaultHeight; // lineCount height is set in minRowHeight state in grid_row_body - } - if (heightOption.height) { - return Math.max(heightOption.height, defaultHeight); - } + if (isObject(heightOption) && heightOption.height) { + return Math.max(heightOption.height, defaultHeight); } if (heightOption && isNumber(heightOption)) { return Math.max(heightOption, defaultHeight); } + if (isObject(heightOption) && heightOption.lineCount) { + if (isRowHeightOverride) { + return this.getRowHeight(rowIndex!) || defaultHeight; // lineCount overrides are stored in the heights cache + } else { + return defaultHeight; // default lineCount height is set in minRowHeight state in grid_row_body + } + } + if (heightOption === AUTO_HEIGHT && rowIndex != null) { return this.getRowHeight(rowIndex); } @@ -117,15 +128,18 @@ export class RowHeightUtils { return isObject(option) ? option.lineCount : undefined; } - calculateHeightForLineCount(cellRef: HTMLElement, lineCount: number) { + calculateHeightForLineCount( + cellRef: HTMLElement, + lineCount: number, + excludePadding?: boolean + ) { const computedStyles = window.getComputedStyle(cellRef, null); const lineHeight = parseInt(computedStyles.lineHeight, 10); + const contentHeight = Math.ceil(lineCount * lineHeight); - return Math.ceil( - lineCount * lineHeight + - this.styles.paddingTop + - this.styles.paddingBottom - ); + return excludePadding + ? contentHeight + : contentHeight + this.styles.paddingTop + this.styles.paddingBottom; } /** From 242d546f70976193d40b77945076d0314cf67edd Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 11 Nov 2021 14:56:26 -0800 Subject: [PATCH 4/9] Add changelog entry --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 869471edafd..beb8eba1a36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## [`main`](https://github.com/elastic/eui/tree/main) -No public interface changes since `41.1.0`. +**Bug fixes** + +- Fixed an `EuiDataGrid` row height bug for grids that set a default `lineCount` and also used `rowHeights` to set row-specific `lineCount`s ([#5376](https://github.com/elastic/eui/pull/5376)) ## [`41.1.0`](https://github.com/elastic/eui/tree/v41.1.0) From c7410539ce7d10207f1cc994a30079958197f265 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 16 Nov 2021 11:21:50 -0800 Subject: [PATCH 5/9] [PR feedback] revert snippet shenanigans --- .../src/views/datagrid/datagrid_height_options_example.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src-docs/src/views/datagrid/datagrid_height_options_example.js b/src-docs/src/views/datagrid/datagrid_height_options_example.js index d0d9e0158d5..924c0b2838b 100644 --- a/src-docs/src/views/datagrid/datagrid_height_options_example.js +++ b/src-docs/src/views/datagrid/datagrid_height_options_example.js @@ -61,7 +61,9 @@ const rowHeightsSnippet = `rowHeightsOptions = { const rowHeightsFullSnippet = `const rowHeightsOptions = useMemo( () => ({ defaultHeight: 140, -}; + }), + [] +); Date: Tue, 16 Nov 2021 11:42:51 -0800 Subject: [PATCH 6/9] [PR feedback] Fix calculateHeightForLineCount not actually excluding padding --- src/components/datagrid/body/data_grid_cell.test.tsx | 6 ++++++ src/components/datagrid/body/data_grid_cell.tsx | 10 ++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx index 5de27836e6d..a56b436d7f8 100644 --- a/src/components/datagrid/body/data_grid_cell.test.tsx +++ b/src/components/datagrid/body/data_grid_cell.test.tsx @@ -218,6 +218,9 @@ describe('EuiDataGridCell', () => { }); callMethod(component); + expect( + mockRowHeightUtils.calculateHeightForLineCount + ).toHaveBeenCalledWith(expect.any(HTMLElement), 3, false); expect(setRowHeight).toHaveBeenCalled(); }); }); @@ -234,6 +237,9 @@ describe('EuiDataGridCell', () => { }); callMethod(component); + expect( + mockRowHeightUtils.calculateHeightForLineCount + ).toHaveBeenCalledWith(expect.any(HTMLElement), 10, true); expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalled(); expect(setRowHeight).not.toHaveBeenCalled(); }); diff --git a/src/components/datagrid/body/data_grid_cell.tsx b/src/components/datagrid/body/data_grid_cell.tsx index 776da1e0d94..a3d20d06672 100644 --- a/src/components/datagrid/body/data_grid_cell.tsx +++ b/src/components/datagrid/body/data_grid_cell.tsx @@ -199,12 +199,18 @@ export class EuiDataGridCell extends Component< const lineCount = rowHeightUtils?.getLineCount(rowHeightOption); if (lineCount) { + const shouldUseHeightsCache = rowHeightUtils?.isRowHeightOverride( + rowIndex, + rowHeightsOptions + ); + const height = rowHeightUtils!.calculateHeightForLineCount( this.cellContentsRef, - lineCount + lineCount, + shouldUseHeightsCache ); - if (rowHeightUtils?.isRowHeightOverride(rowIndex, rowHeightsOptions)) { + if (shouldUseHeightsCache) { const { columnId, visibleRowIndex } = this.props; rowHeightUtils?.setRowHeight( rowIndex, From e7c6ed18a304b5b4f6c86055d2f51186305cc7a3 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 16 Nov 2021 11:43:36 -0800 Subject: [PATCH 7/9] [REVERT ME] QA row heights on an easier-to-see example --- src-docs/src/views/datagrid/row_line_height.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src-docs/src/views/datagrid/row_line_height.tsx b/src-docs/src/views/datagrid/row_line_height.tsx index d79b9b86326..17b1c450958 100644 --- a/src-docs/src/views/datagrid/row_line_height.tsx +++ b/src-docs/src/views/datagrid/row_line_height.tsx @@ -141,6 +141,11 @@ export default () => { defaultHeight: { lineCount: 3, }, + rowHeights: { + 0: { lineCount: 1 }, + 1: { lineCount: 2 }, + 3: { lineCount: 4 }, + }, lineHeight: '2em', }), [] From e90193fad0722f10dc67b26d0e2838a2dc3b012d Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 16 Nov 2021 15:48:19 -0800 Subject: [PATCH 8/9] Revert "[REVERT ME] test lineCount override fix" This reverts commit 9a0a65ba456347480433e1752737a16219284a91. --- src-docs/src/views/datagrid/row_height_fixed.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src-docs/src/views/datagrid/row_height_fixed.tsx b/src-docs/src/views/datagrid/row_height_fixed.tsx index ab5635dbfae..91df500a1ca 100644 --- a/src-docs/src/views/datagrid/row_height_fixed.tsx +++ b/src-docs/src/views/datagrid/row_height_fixed.tsx @@ -184,11 +184,13 @@ export default () => { // matches the snippet example const rowHeightsOptions = useMemo( () => ({ - defaultHeight: { lineCount: 3 }, + defaultHeight: 140, rowHeights: { - 1: { lineCount: 5 }, - 4: { lineCount: 1 }, - 5: { lineCount: 10 }, + 1: { + lineCount: 5, + }, + 4: 200, + 5: 80, }, }), [] From b69ae1df4607c515ffe0bb671e826acea6db9bd1 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 16 Nov 2021 15:48:28 -0800 Subject: [PATCH 9/9] Revert "[REVERT ME] QA row heights on an easier-to-see example" This reverts commit e7c6ed18a304b5b4f6c86055d2f51186305cc7a3. --- src-docs/src/views/datagrid/row_line_height.tsx | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src-docs/src/views/datagrid/row_line_height.tsx b/src-docs/src/views/datagrid/row_line_height.tsx index 17b1c450958..d79b9b86326 100644 --- a/src-docs/src/views/datagrid/row_line_height.tsx +++ b/src-docs/src/views/datagrid/row_line_height.tsx @@ -141,11 +141,6 @@ export default () => { defaultHeight: { lineCount: 3, }, - rowHeights: { - 0: { lineCount: 1 }, - 1: { lineCount: 2 }, - 3: { lineCount: 4 }, - }, lineHeight: '2em', }), []