Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand Down
7 changes: 1 addition & 6 deletions src/components/datagrid/__mocks__/row_height_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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],
}
Expand Down Expand Up @@ -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],
}
Expand Down
54 changes: 34 additions & 20 deletions src/components/datagrid/body/data_grid_cell.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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> });
});
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand Down
79 changes: 20 additions & 59 deletions src/components/datagrid/body/data_grid_cell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
}

Comment on lines -289 to -304
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick walkthrough of changes:

  1. 0eab3ab is the primary workaround @chandlerprall proposed and @VladLasitsa had in his draft PR

  2. (this code diff) a553005 removes height check logic from shouldComponentUpdate as well. I figured if we aren't checking for same height, it makes sense to remove this code also to reduce extra updates. When I tested with auto height I did not see any issues or problems with height not updating when it should (I tested sorting, density, and hiding/reordering columns).

  3. Some optional proposed refactors: 6b870fd

    • I DRY'd it out so that both the ResizeObserver and props change could call the same method. This leads to some repetition with the height being obtained twice in the observer, but it will not be an issue for long as I'll be refactoring/removing the cell wrapper observer in the row height switcher PR (it does not appear to be doing anything)
    • I refactored rowHeightUtils.isAutoHeight to accept an undefined rowHeightsOptions

return false;
}

Expand All @@ -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();
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/datagrid/data_grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading