[EuiDataGrid] Fix grids that set a defaultHeight with lineCount and rowHeights overrides with lineCount

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 - Added `aria-label` and `aria-labelledby` props to `EuiComboBox` ([#5360](https://github.com/elastic/eui/issues/5360))
 
+**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)
 
 - Added `layout` and `footer` props to `EuiEmptyPrompt` ([#5275](https://github.com/elastic/eui/pull/5275))

src-docs/src/views/datagrid/datagrid_height_options_example.js

@@ -87,12 +87,6 @@ const rowHeightsFullSnippet = `const rowHeightsOptions = useMemo(
 const autoRowHeightsSnippet = `// the demo below matches this snippet
 rowHeightsOptions = {
   defaultHeight: 'auto', // all rows will automatically adjust the height except rows defined in 'rowHeights'
-  rowHeights: {
-    1: {
-      lineCount: 5, // row at index 1 will show 5 lines
-    },
-    4: 140, // row at index 4 will adjust the height to 140px
-  },
 }
 
 // you can also automatically adjust the height for a specific row

src-docs/src/views/datagrid/row_height_auto.tsx

@@ -185,12 +185,6 @@ export default () => {
   const rowHeightsOptions = useMemo(
     () => ({
       defaultHeight: 'auto' as const,
-      rowHeights: {
-        1: {
-          lineCount: 5,
-        },
-        4: 140,
-      },
     }),
     []
   );

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);

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],

src/components/datagrid/body/data_grid_body.tsx

@@ -532,7 +532,8 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
         height = rowHeightUtils.getCalculatedHeight(
           rowHeightOption,
           minRowHeight,
-          correctRowIndex
+          correctRowIndex,
+          rowHeightUtils.isRowHeightOverride(correctRowIndex, rowHeightsOptions)
         );
       }
 

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,43 @@ 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(
+            mockRowHeightUtils.calculateHeightForLineCount
+          ).toHaveBeenCalledWith(expect.any(HTMLElement), 3, false);
+          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.calculateHeightForLineCount
+          ).toHaveBeenCalledWith(expect.any(HTMLElement), 10, true);
+          expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalled();
+          expect(setRowHeight).not.toHaveBeenCalled();
+        });
       });
 
       it('does nothing if cell height is not set to lineCount', () => {

src/components/datagrid/body/data_grid_cell.tsx

@@ -199,12 +199,28 @@ 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
       );
 
-      this.props.setRowHeight(height);
+      if (shouldUseHeightsCache) {
+        const { columnId, visibleRowIndex } = this.props;
+        rowHeightUtils?.setRowHeight(
+          rowIndex,
+          columnId,
+          height,
+          visibleRowIndex
+        );
+      } else {
+        this.props.setRowHeight(height);
+      }
     }
   };
 

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
+      });
     });
   });
 

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;
   }
 
   /**