Fix stale cellPadding updates/calculations

- remove the cached styles and just use `window.getComputedStyles` - which causes a reflow, but since we need it for lineHeight in any case we might as well keep using it 🤷
elastic · Sep 14, 2024 · 52bb4d5 · 52bb4d5
1 parent 0d8b6ed
commit 52bb4d5
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 104 deletions.
diff --git a/packages/eui/src/components/datagrid/utils/__mocks__/row_heights.ts b/packages/eui/src/components/datagrid/utils/__mocks__/row_heights.ts
@@ -23,7 +23,6 @@ export const RowHeightUtils = jest.fn().mockImplementation(() => {
   const rowHeightUtils = new ActualRowHeightUtils();
 
   const rowHeightUtilsMock: RowHeightUtilsPublicAPI = {
-    cacheStyles: jest.fn(),
     getHeightType: jest.fn(rowHeightUtils.getHeightType),
     isAutoHeight: jest.fn(() => false),
     setRowHeight: jest.fn(),

diff --git a/packages/eui/src/components/datagrid/utils/row_heights.test.ts b/packages/eui/src/components/datagrid/utils/row_heights.test.ts
@@ -159,47 +159,6 @@ describe('RowHeightUtils', () => {
     });
   });
 
-  describe('styles utils', () => {
-    describe('cacheStyles', () => {
-      const mockCellPaddingsMap = {
-        s: '4px',
-        m: '6px',
-        l: '8px',
-      };
-
-      it('stores a styles instance variable based on the grid density', () => {
-        Object.entries(mockCellPaddingsMap).forEach(
-          ([densitySize, paddingStyle]) => {
-            const expectedPadding = parseFloat(paddingStyle);
-
-            rowHeightUtils.cacheStyles(
-              { cellPadding: densitySize as any },
-              mockCellPaddingsMap
-            );
-            // @ts-ignore this var is private, but we're inspecting it for the sake of the unit test
-            expect(rowHeightUtils.styles).toEqual({
-              paddingTop: expectedPadding,
-              paddingBottom: expectedPadding,
-            });
-          }
-        );
-      });
-
-      it('falls back to m-sized cellPadding if gridStyle.cellPadding is undefined', () => {
-        rowHeightUtils.cacheStyles(
-          { cellPadding: undefined },
-          mockCellPaddingsMap
-        );
-
-        // @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('getHeightType', () => {
     it('returns a string enum based on rowHeightsOptions', () => {
       expect(rowHeightUtils.getHeightType(undefined)).toEqual('default');
@@ -242,13 +201,14 @@ describe('RowHeightUtils', () => {
     describe('calculateHeightForLineCount', () => {
       let getComputedStyleSpy: jest.SpyInstance;
       const cell = document.createElement('div');
-      const mockCellPaddingsMap = { s: '', m: '6px', l: '' };
 
       beforeEach(() => {
-        rowHeightUtils.cacheStyles({ cellPadding: 'm' }, mockCellPaddingsMap);
         getComputedStyleSpy = jest
           .spyOn(window, 'getComputedStyle')
-          .mockReturnValue({ lineHeight: '24px' } as CSSStyleDeclaration);
+          .mockReturnValue({
+            lineHeight: '24px',
+            paddingTop: '6px',
+          } as CSSStyleDeclaration);
       });
 
       afterEach(() => {
@@ -643,23 +603,6 @@ describe('useRowHeightUtils', () => {
     expect(requestAnimationFrameSpy).toHaveBeenCalledTimes(3);
   });
 
-  it('updates internal cached styles whenever gridStyle.cellPadding changes', () => {
-    const { result, rerender } = renderHook(useRowHeightUtils, {
-      initialProps: mockArgs,
-    });
-
-    rerender({
-      ...mockArgs,
-      gridStyles: { ...startingStyles, cellPadding: 's' },
-    });
-
-    // @ts-ignore - intentionally inspecting private var for test
-    expect(result.current.styles).toEqual({
-      paddingTop: 4,
-      paddingBottom: 4,
-    });
-  });
-
   it('prunes columns from the row heights cache if a column is hidden', () => {
     const { result, rerender } = renderHook(useRowHeightUtils, {
       initialProps: mockArgs,

diff --git a/packages/eui/src/components/datagrid/utils/row_heights.ts b/packages/eui/src/components/datagrid/utils/row_heights.ts
@@ -15,11 +15,7 @@ import {
   useState,
 } from 'react';
 import { GridOnItemsRenderedProps } from 'react-window';
-import {
-  useForceRender,
-  useLatest,
-  useEuiMemoizedStyles,
-} from '../../../services';
+import { useForceRender, useLatest } from '../../../services';
 import { isNumber, isObject } from '../../../services/predicate';
 import {
   EuiDataGridColumn,
@@ -29,7 +25,6 @@ import {
   EuiDataGridStyle,
   ImperativeGridApi,
 } from '../data_grid_types';
-import { euiDataGridVariables } from '../data_grid.styles';
 import { DataGridSortedContext } from './sorting';
 
 const AUTO_HEIGHT = 'auto';
@@ -84,31 +79,6 @@ export class RowHeightUtils {
     return defaultHeight;
   }
 
-  /**
-   * Styles utils
-   */
-
-  private styles: {
-    paddingTop: number;
-    paddingBottom: number;
-  } = {
-    paddingTop: 0,
-    paddingBottom: 0,
-  };
-
-  cacheStyles(
-    gridStyles: EuiDataGridStyle,
-    cellPaddingsMap: ReturnType<typeof euiDataGridVariables>['cellPadding']
-  ) {
-    const paddingSize = gridStyles.cellPadding ?? 'm';
-    const padding = parseFloat(cellPaddingsMap[paddingSize]);
-
-    this.styles = {
-      paddingTop: padding,
-      paddingBottom: padding,
-    };
-  }
-
   /**
    * Height types
    */
@@ -139,8 +109,10 @@ export class RowHeightUtils {
     const computedStyles = window.getComputedStyle(cellRef, null);
     const lineHeight = parseInt(computedStyles.lineHeight, 10);
     const contentHeight = Math.ceil(lineCount * lineHeight);
+    const padding = parseInt(computedStyles.paddingTop, 10);
 
-    return contentHeight + this.styles.paddingTop + this.styles.paddingBottom;
+    // Assumes both padding-top and bottom are the same
+    return contentHeight + padding * 2;
   }
 
   /**
@@ -328,7 +300,6 @@ export class RowHeightVirtualizationUtils extends RowHeightUtils {
 export const useRowHeightUtils = ({
   virtualization,
   rowHeightsOptions,
-  gridStyles,
   columns,
 }: {
   virtualization?:
@@ -375,15 +346,6 @@ export const useRowHeightUtils = ({
     forceRenderRef,
   ]);
 
-  // Re-cache styles whenever grid density changes
-  const styleVars = useEuiMemoizedStyles(euiDataGridVariables);
-  useEffect(() => {
-    rowHeightUtils.cacheStyles(
-      { cellPadding: gridStyles.cellPadding },
-      styleVars.cellPadding
-    );
-  }, [gridStyles.cellPadding, rowHeightUtils, styleVars.cellPadding]);
-
   // Update row heights map to remove hidden columns whenever orderedVisibleColumns change
   useEffect(() => {
     rowHeightUtils.pruneHiddenColumnHeights(columns);