[EuiDataGrid] Fix row height not updating dynamically when set to `au…

…to` (#5281) * Remove isHeightSame check * Remove same height check from shouldComponentUpdate - if we're not checking for sameHeight in componentDidUpdate, we also likely don't need this anymore, and all dynamic user changes appear to work without this code * [Proposed refactors] - Rename `recalculateRowHeight` to `setAutoRowHeight` - DRY out setAutoRowHeight to be reusable by both the resize observer and during props update (NB: This leads to some repetition with the height being obtained twice, but will not be an issue shortly as I'll be refactoring/removing the cell wrapper observer in a future PR) - Refactor rowHeightUtils.isAutoHeight to accept an undefined rowHeightsOptions * Improve unit tests * Add changelog entry * [PR feedback] Fix `isAutoHeight` false positive - occurs if the `defaultHeight` is auto but a specific `rowHeights` line override is not auto TODO: Write a unit test for this in the future * [PR feedback/Discover testing] Trigger component update when rowHeightsOptions changes * [cleanup] Remove now-unused compareHeights util + clean up mock rowHeightUtils as well * [revert] rename setAutoRowHeight back to recalculateRowHeight - lineCount in #5284 will shortly be using the row height cache in addition to auto, so I'm making the fn name less specific again * Add enqueue/timeout to recalculateRowHeight updates - to avoid race conditions with cache being cleared * Remove clearHeightsCache completely * Unset row+column height values on unmount (#3) * Add unit test for new unsetRowHeight + add optional isAutoHeight check * Remove hidden columns from row height cache * Early return in getRowHeight Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
elastic · Oct 28, 2021 · 8563fbe · 8563fbe
1 parent 7c77e77
commit 8563fbe
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 117 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 - Fixed `mobileOptions.truncateText` from getting overridden by `truncateText` in `EuiTableRowCell` ([#5283](https://github.com/elastic/eui/pull/5283))
 - Fixed issue with dynamic row counts in `EuiDataGrid` ([#5313](https://github.com/elastic/eui/pull/5313))
 - Fixed `EuiDataGrid`'s expanded density not increasing font sizes on Amsterdam ([#5320](https://github.com/elastic/eui/pull/5320))
+- Fixed `EuiDataGrid` to dynamically update row heights when set to `auto` ([#5281](https://github.com/elastic/eui/pull/5281))
 
 **Theme: Amsterdam**
 

diff --git a/src/components/datagrid/__mocks__/row_height_utils.ts b/src/components/datagrid/__mocks__/row_height_utils.ts
@@ -12,7 +12,6 @@ import { RowHeightUtils } from '../row_height_utils';
 
 export const mockRowHeightUtils = ({
   computeStylesForGridCell: jest.fn(),
-  clearHeightsCache: jest.fn(),
   setGrid: jest.fn(),
   getStylesForCell: jest.fn(() => ({
     wordWrap: 'break-word',
@@ -22,13 +21,9 @@ export const mockRowHeightUtils = ({
   isDefinedHeight: jest.fn(() => true),
   isAutoHeight: jest.fn(() => false),
   setRowHeight: jest.fn(),
+  pruneHiddenColumnHeights: jest.fn(),
   getRowHeight: jest.fn(() => 32),
-  getFont: jest.fn(() => null),
   getComputedCellStyles: jest.fn(() => {}),
-  compareHeights: jest.fn(
-    (currentRowHeight: number, cachedRowHeight: number) =>
-      currentRowHeight === cachedRowHeight
-  ),
   getCalculatedHeight: jest.fn(
     (heightOption: EuiDataGridRowHeightOption, defaultHeight: number) => {
       if (typeof heightOption === 'object') {

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
@@ -10,16 +10,14 @@ exports[`EuiDataGridCell renders 1`] = `
   renderCellValue={[Function]}
   rowHeightUtils={
     Object {
-      "clearHeightsCache": [MockFunction],
-      "compareHeights": [MockFunction],
       "computeStylesForGridCell": [MockFunction],
       "getCalculatedHeight": [MockFunction],
       "getComputedCellStyles": [MockFunction],
-      "getFont": [MockFunction],
       "getRowHeight": [MockFunction],
       "getStylesForCell": [MockFunction],
       "isAutoHeight": [MockFunction],
       "isDefinedHeight": [MockFunction],
+      "pruneHiddenColumnHeights": [MockFunction],
       "setGrid": [MockFunction],
       "setRowHeight": [MockFunction],
     }
@@ -59,16 +57,14 @@ exports[`EuiDataGridCell renders 1`] = `
           renderCellValue={[Function]}
           rowHeightUtils={
             Object {
-              "clearHeightsCache": [MockFunction],
-              "compareHeights": [MockFunction],
               "computeStylesForGridCell": [MockFunction],
               "getCalculatedHeight": [MockFunction],
               "getComputedCellStyles": [MockFunction],
-              "getFont": [MockFunction],
               "getRowHeight": [MockFunction],
               "getStylesForCell": [MockFunction],
               "isAutoHeight": [MockFunction],
               "isDefinedHeight": [MockFunction],
+              "pruneHiddenColumnHeights": [MockFunction],
               "setGrid": [MockFunction],
               "setRowHeight": [MockFunction],
             }

diff --git a/src/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx
@@ -9,8 +9,8 @@
 import React from 'react';
 import { mount, ReactWrapper } from 'enzyme';
 import { keys } from '../../../services';
-
 import { mockRowHeightUtils } from '../__mocks__/row_height_utils';
+
 import { EuiDataGridCell } from './data_grid_cell';
 
 describe('EuiDataGridCell', () => {
@@ -108,6 +108,9 @@ describe('EuiDataGridCell', () => {
         it('width', () => {
           component.setProps({ width: 30 });
         });
+        it('rowHeightsOptions', () => {
+          component.setProps({ rowHeightsOptions: { defaultHeight: 'auto' } });
+        });
         it('renderCellValue', () => {
           component.setProps({ renderCellValue: () => <div>test</div> });
         });
@@ -148,16 +151,6 @@ describe('EuiDataGridCell', () => {
           component.setState({ disableCellTabIndex: true });
         });
       });
-
-      it('when cell height changes', () => {
-        Object.defineProperty(HTMLElement.prototype, 'offsetHeight', {
-          configurable: true,
-          value: 10,
-        });
-        const getRowHeight = jest.fn(() => 20);
-
-        component.setProps({ getRowHeight });
-      });
     });
 
     it('should not update for prop/state changes not specified above', () => {
@@ -167,19 +160,40 @@ describe('EuiDataGridCell', () => {
   });
 
   describe('componentDidUpdate', () => {
-    it('recalculates row height on every update', () => {
-      const { isAutoHeight, setRowHeight } = mockRowHeightUtils;
-      (isAutoHeight as jest.Mock).mockImplementation(() => true);
+    describe('recalculateRowHeight', () => {
+      beforeEach(() => {
+        (mockRowHeightUtils.setRowHeight as jest.Mock).mockClear();
+      });
+      afterEach(() => {
+        (mockRowHeightUtils.isAutoHeight as jest.Mock).mockRestore();
+      });
 
-      const component = mountEuiDataGridCellWithContext({
-        rowHeightsOptions: { defaultHeight: 'auto' },
-        getRowHeight: jest.fn(() => 50),
+      const triggerUpdate = (component: ReactWrapper) =>
+        component.setProps({ rowIndex: 2 });
+
+      it('sets the row height cache with cell heights on update', () => {
+        (mockRowHeightUtils.isAutoHeight as jest.Mock).mockReturnValue(true);
+
+        const component = mountEuiDataGridCellWithContext({
+          rowHeightsOptions: { defaultHeight: 'auto' },
+          getRowHeight: jest.fn(() => 50),
+        });
+
+        triggerUpdate(component);
+        expect(mockRowHeightUtils.setRowHeight).toHaveBeenCalled();
       });
 
-      component.setProps({ rowIndex: 2 }); // Trigger any update
-      expect(setRowHeight).toHaveBeenCalled();
+      it('does not update the cache if cell height is not auto', () => {
+        (mockRowHeightUtils.isAutoHeight as jest.Mock).mockReturnValue(false);
 
-      (isAutoHeight as jest.Mock).mockRestore();
+        const component = mountEuiDataGridCellWithContext({
+          rowHeightsOptions: { defaultHeight: 34 },
+          getRowHeight: jest.fn(() => 50),
+        });
+
+        triggerUpdate(component);
+        expect(mockRowHeightUtils.setRowHeight).not.toHaveBeenCalled();
+      });
     });
 
     it('resets cell props when the cell columnId changes', () => {

diff --git a/src/components/datagrid/body/data_grid_cell.tsx b/src/components/datagrid/body/data_grid_cell.tsx
@@ -194,32 +194,24 @@ export class EuiDataGridCell extends Component<
     }
   };
 
-  recalculateRowHeight() {
-    const cellRef = this.cellRef.current;
-    const { getRowHeight, rowHeightUtils, rowHeightsOptions } = this.props;
-
-    if (cellRef && getRowHeight && rowHeightUtils && rowHeightsOptions) {
-      const { rowIndex, colIndex, visibleRowIndex } = this.props;
+  recalculateRowHeight = () => {
+    const { rowHeightUtils, rowHeightsOptions, rowIndex } = this.props;
+    if (
+      this.cellContentsRef &&
+      rowHeightUtils &&
+      rowHeightUtils.isAutoHeight(rowIndex, rowHeightsOptions)
+    ) {
+      const { columnId, visibleRowIndex } = this.props;
+      const rowHeight = this.cellContentsRef.offsetHeight;
 
-      const isAutoHeight = rowHeightUtils.isAutoHeight(
+      rowHeightUtils.setRowHeight(
         rowIndex,
-        rowHeightsOptions
-      );
-      const isHeightSame = rowHeightUtils.compareHeights(
-        cellRef.offsetHeight,
-        getRowHeight(rowIndex)
+        columnId,
+        rowHeight,
+        visibleRowIndex
       );
-
-      if (isAutoHeight && !isHeightSame) {
-        rowHeightUtils.setRowHeight(
-          rowIndex,
-          colIndex,
-          this.cellContentsRef?.offsetHeight,
-          visibleRowIndex
-        );
-      }
     }
-  }
+  };
 
   componentDidMount() {
     this.unsubscribeCell = this.context.onFocusUpdate(
@@ -261,6 +253,8 @@ export class EuiDataGridCell extends Component<
     if (nextProps.columnId !== this.props.columnId) return true;
     if (nextProps.columnType !== this.props.columnType) return true;
     if (nextProps.width !== this.props.width) return true;
+    if (nextProps.rowHeightsOptions !== this.props.rowHeightsOptions)
+      return true;
     if (nextProps.renderCellValue !== this.props.renderCellValue) return true;
     if (nextProps.interactiveCellId !== this.props.interactiveCellId)
       return true;
@@ -286,22 +280,6 @@ export class EuiDataGridCell extends Component<
     if (nextState.disableCellTabIndex !== this.state.disableCellTabIndex)
       return true;
 
-    // check if we should update cell because height was changed
-    if (
-      this.cellRef.current &&
-      nextProps.getRowHeight &&
-      nextProps.rowHeightUtils
-    ) {
-      if (
-        !nextProps.rowHeightUtils?.compareHeights(
-          this.cellRef.current.offsetHeight,
-          nextProps.getRowHeight(nextProps.rowIndex)
-        )
-      ) {
-        return true;
-      }
-    }
-
     return false;
   }
 
@@ -311,27 +289,10 @@ export class EuiDataGridCell extends Component<
 
   setCellContentsRef = (ref: HTMLDivElement | null) => {
     this.cellContentsRef = ref;
-    const { rowHeightUtils, rowHeightsOptions, rowIndex } = this.props;
-    if (
-      hasResizeObserver &&
-      rowHeightUtils &&
-      rowHeightsOptions &&
-      rowHeightUtils.isAutoHeight(rowIndex, rowHeightsOptions)
-    ) {
-      if (ref) {
-        const { colIndex, visibleRowIndex } = this.props;
-
-        const setRowHeight = (rowHeight: number) =>
-          rowHeightUtils.setRowHeight(
-            rowIndex,
-            colIndex,
-            rowHeight,
-            visibleRowIndex
-          );
-        this.contentObserver = this.observeHeight(ref, setRowHeight);
-      } else if (this.contentObserver) {
-        this.contentObserver.disconnect();
-      }
+    if (ref && hasResizeObserver) {
+      this.contentObserver = this.observeHeight(ref, this.recalculateRowHeight);
+    } else if (this.contentObserver) {
+      this.contentObserver.disconnect();
     }
     this.preventTabbing();
   };

diff --git a/src/components/datagrid/data_grid.tsx b/src/components/datagrid/data_grid.tsx
@@ -667,8 +667,8 @@ export const EuiDataGrid: FunctionComponent<EuiDataGridProps> = (props) => {
   const rowHeightUtils = useMemo(() => new RowHeightUtils(), []);
 
   useEffect(() => {
-    rowHeightUtils.clearHeightsCache();
-  }, [orderedVisibleColumns, rowHeightsOptions, rowHeightUtils]);
+    rowHeightUtils.pruneHiddenColumnHeights(orderedVisibleColumns);
+  }, [rowHeightUtils, orderedVisibleColumns]);
 
   const [contentRef, setContentRef] = useState<HTMLDivElement | null>(null);