Skip to content

[EuiDataGrid] Fix row height tests and mocks #6050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
weltenwort merged 8 commits into from
Jul 19, 2022
Merged

[EuiDataGrid] Fix row height tests and mocks #6050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d6091a3
Fix row height tests and mocks
weltenwort Jul 13, 2022
24f6bce
Restore accidentally removed test case
weltenwort Jul 13, 2022
75b8e28
Update src/components/datagrid/utils/row_heights.test.ts
weltenwort Jul 14, 2022
9efe690
Merge branch 'main' into data-grid-fix-row-height-tests
weltenwort Jul 18, 2022
6c2c170
Clean up forceRender usage and RowHeightUtils instantiation
weltenwort Jul 18, 2022
fd36097
Deduplicate grid API type
weltenwort Jul 18, 2022
c6ac32b
Comment and test the useLatest hook
weltenwort Jul 18, 2022
b5c152b
Fix typo in comment
weltenwort Jul 18, 2022
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
23 changes: 14 additions & 9 deletions src/components/datagrid/body/data_grid_body.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,22 @@
import React from 'react';
import { mount, render, shallow } from 'enzyme';

import { mockRowHeightUtils } from '../utils/__mocks__/row_heights';
import { RowHeightUtils } from '../utils/__mocks__/row_heights';
import { schemaDetectors } from '../utils/data_grid_schema';

import { EuiDataGridBody, Cell } from './data_grid_body';

describe('EuiDataGridBody', () => {
const gridRef = {
current: {
resetAfterColumnIndex: jest.fn(),
resetAfterRowIndex: jest.fn(),
} as any,
};
const gridItemsRendered = { current: null };
const rerenderGridBodyRef = { current: null };
const rowHeightUtils = new RowHeightUtils(gridRef, rerenderGridBodyRef);
Copy link
Contributor

@cee-chen cee-chen Jul 13, 2022

Choose a reason for hiding this comment

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

[super optional] if you wanted to leave these required props inline, you could also set rowHeightUtils afterwards on line 60 by doing

const requiredProps = {
  // ...
  gridRef: {
    // etc,
  },
  gridItemsRendered: // etc,
  // ...
};
requiredProps.rowHeightUtils = newRowHeightUtils(requiredProps.gridRef, { current: null });

That is a relatively minor preference on my part to reduce test file setup cruft as much as possible (or as much as isn't actually relevant to what we're testing - and in this file row heights aren't really being tested).

Copy link
Member Author

@weltenwort weltenwort Jul 14, 2022

Choose a reason for hiding this comment

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

If that's the style you prefer, I can change it. I'd recommend to treat values immutably as often as possible, though. It usually reduces cognitive load on the reader and allows for more compiler checks and JIT optimizations.

Copy link
Contributor

@cee-chen cee-chen Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Super interesting!! (I love code discussions like this BTW, so you'll have to forgive me for overindulging 😄)

allows for more compiler checks and JIT optimizations.

TIL 🎓 I had thought that instantiating fewer consts/vars vs. just 1 obj would be 'better' in terms of optimization. Compiler checks makes sense though. I know we're splitting hairs at this point though in terms of microperf, but definitely food for me to think about!

It usually reduces cognitive load on the reader

I definitely acknowledge this is very subjective, but my personal 2c is that more cruft/setup at the top of the file is harder for me to read / more cognitive load than otherwise. I prefer to jump straight into the core purpose/logic of the file when possible to try and grasp its intentions. When I come across a var I'll tend to store it in my short-term memory assuming it'll be re-used later and I'll want to try and connect the var definition/reference to where it's being used, so more vars = more things flying around my head to try and remember. So for the test files if I start by seeing a single const requiredProps = ... declaration, I get a sense of "oh these are just the props needed to get the test started", and I'll collapse that obj or skim/skip past it, whereas if I see other vars defined I'll be like "this seems important/will come into play later, I'll try to remember it individually".

Like I said, very much subjective though, forgive my rambling! My comment definitely was optional/not a change request so if you prefer it this way, feel free to leave it


const requiredProps = {
headerIsInteractive: true,
rowCount: 1,
Expand All @@ -39,17 +49,12 @@ describe('EuiDataGridBody', () => {
setVisibleColumns: jest.fn(),
switchColumnPos: jest.fn(),
schemaDetectors,
rowHeightUtils: mockRowHeightUtils,
rowHeightUtils,
isFullScreen: false,
gridStyles: {},
gridWidth: 300,
gridRef: {
current: {
resetAfterColumnIndex: jest.fn(),
resetAfterRowIndex: jest.fn(),
} as any,
},
gridItemsRendered: {} as any,
gridRef,
gridItemsRendered,
wrapperRef: { current: document.createElement('div') },
};

Expand Down
2 changes: 1 addition & 1 deletion src/components/datagrid/body/data_grid_body.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
* Heights
*/
const rowHeightUtils = useRowHeightUtils({
gridRef: gridRef.current,
gridRef,
gridStyles,
columns,
rowHeightsOptions,
Expand Down
7 changes: 6 additions & 1 deletion src/components/datagrid/body/data_grid_cell.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@
import React, { useEffect } from 'react';
import { mount, render, ReactWrapper } from 'enzyme';
import { keys } from '../../../services';
import { mockRowHeightUtils } from '../utils/__mocks__/row_heights';
import { RowHeightUtils } from '../utils/__mocks__/row_heights';
import { mockFocusContext } from '../utils/__mocks__/focus_context';
import { DataGridFocusContext } from '../utils/focus';

import { EuiDataGridCell } from './data_grid_cell';

describe('EuiDataGridCell', () => {
const mockRowHeightUtils = new RowHeightUtils(
{ current: null },
{ current: null }
);

const mockPopoverContext = {
popoverIsOpen: false,
cellLocation: { rowIndex: 0, colIndex: 0 },
Expand Down
2 changes: 2 additions & 0 deletions src/components/datagrid/data_grid_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import { RowHeightUtils } from './utils/row_heights';
import { IconType } from '../icon';
import { EuiTokenProps } from '../token';

// since react-window doesn't export a type with the imperative api only we can
// use this to omit the react-specific class component methods
export type ImperativeGridApi = Omit<Grid, keyof Component>;

export interface EuiDataGridToolbarProps {
Expand Down
54 changes: 33 additions & 21 deletions src/components/datagrid/utils/__mocks__/row_heights.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,38 @@

import { RowHeightUtils as ActualRowHeightUtils } from '../row_heights';

const actual = new ActualRowHeightUtils();
type RowHeightUtilsPublicAPI = Pick<
ActualRowHeightUtils,
keyof ActualRowHeightUtils
>;

export const mockRowHeightUtils = ({
cacheStyles: jest.fn(),
setGrid: jest.fn(),
getStylesForCell: jest.fn(() => ({
wordWrap: 'break-word',
wordBreak: 'break-word',
flexGrow: 1,
})),
isAutoHeight: jest.fn(() => false),
setRowHeight: jest.fn(),
pruneHiddenColumnHeights: jest.fn(),
getRowHeight: jest.fn(() => 32),
getRowHeightOption: jest.fn(actual.getRowHeightOption),
getCalculatedHeight: jest.fn(actual.getCalculatedHeight),
getLineCount: jest.fn(actual.getLineCount),
calculateHeightForLineCount: jest.fn(() => 50),
isRowHeightOverride: jest.fn(actual.isRowHeightOverride),
setRerenderGridBody: jest.fn(),
} as unknown) as ActualRowHeightUtils;
export const RowHeightUtils = jest
.fn<
ActualRowHeightUtils,
ConstructorParameters<typeof ActualRowHeightUtils>
>()
.mockImplementation((...args) => {
const rowHeightUtils = new ActualRowHeightUtils(...args);

export const RowHeightUtils = jest.fn(() => mockRowHeightUtils);
const rowHeightUtilsMock: RowHeightUtilsPublicAPI = {
cacheStyles: jest.fn(),
getStylesForCell: jest.fn(() => ({
wordWrap: 'break-word',
wordBreak: 'break-word',
flexGrow: 1,
})),
isAutoHeight: jest.fn(() => false),
setRowHeight: jest.fn(),
pruneHiddenColumnHeights: jest.fn(),
getRowHeight: jest.fn(() => 32),
getRowHeightOption: jest.fn(rowHeightUtils.getRowHeightOption),
getCalculatedHeight: jest.fn(rowHeightUtils.getCalculatedHeight),
getLineCount: jest.fn(rowHeightUtils.getLineCount),
calculateHeightForLineCount: jest.fn(() => 50),
isRowHeightOverride: jest.fn(rowHeightUtils.isRowHeightOverride),
resetRow: jest.fn(),
resetGrid: jest.fn(),
};

return (rowHeightUtilsMock as any) as ActualRowHeightUtils;
});
Loading