Skip to content
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.

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

[EuiDataGrid] Handle exposed ref APIs potentially pointing to invalid, off-page, or out of view cells #5572

Merged
merged 10 commits into from
Feb 1, 2022
30 changes: 25 additions & 5 deletions src-docs/src/views/datagrid/datagrid_ref_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const DataGridRefExample = {
{
source: [
{
type: GuideSectionTypes.JS,
type: GuideSectionTypes.TSX,
code: dataGridRefSource,
},
],
Expand Down Expand Up @@ -63,10 +63,30 @@ export const DataGridRefExample = {
<EuiSpacer size="m" />
</li>
<li>
<p>
<EuiCode>openCellPopover({'{ rowIndex, colIndex }'})</EuiCode> -
opens the specified cell&apos;s popover contents.
</p>
<EuiCode>openCellPopover({'{ rowIndex, colIndex }'})</EuiCode> -
opens the specified cell&apos;s popover contents.
<EuiSpacer size="s" />
<EuiCallOut iconType="mapMarker" title="Handling cell location">
When using <EuiCode>setFocusedCell</EuiCode> or{' '}
<EuiCode>openCellPopover</EuiCode>, keep in mind:
<ul>
<li>
colIndex is affected by the user reordering or hiding
columns.
</li>
<li>
If the passed cell indices are outside the data grid&apos;s
total row count or visible column count, an error will be
thrown.
</li>
<li>
If the data grid is paginated or sorted, the grid will
handle automatically finding specified row index&apos;s
correct location for you.
</li>
</ul>
</EuiCallOut>
<EuiSpacer size="m" />
</li>
<li>
<p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { useCallback, useMemo, useState, useRef } from 'react';
// @ts-ignore - faker does not have type declarations
import { fake } from 'faker';
Comment on lines 1 to 3
Copy link
Member Author

Choose a reason for hiding this comment

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

Just curious: is there a quick or easy way of making it so this // @ts-ignore doesn't show up in our Demo TS tab or CodeSandbox?

Screen Shot 2022-01-27 at 2 02 02 PM

I'm guessing no unless we switch to a lib with type declarations? (Might be worth doing anyway, what with the whole faker kerfluffle 🤷‍♀️)

Copy link
Contributor

Choose a reason for hiding this comment

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

I keep promising Elizabet that I would make the snapshot of github data we use in the height examples available to other examples, which would hopefully help phase out faker.

Probably not worth putting any time into removing those comments, they may even be beneficial to consumers by indicating what part of an example may need some extra attention when copying.


import {
Expand All @@ -9,15 +10,16 @@ import {
EuiFieldNumber,
EuiButton,
EuiDataGrid,
EuiDataGridRefProps,
EuiModal,
EuiModalBody,
EuiModalFooter,
EuiModalHeader,
EuiModalHeaderTitle,
EuiText,
} from '../../../../src/components/';
} from '../../../../src/components';

const raw_data = [];
const raw_data: Array<{ [key: string]: string }> = [];
for (let i = 1; i < 100; i++) {
raw_data.push({
name: fake('{{name.lastName}}, {{name.firstName}}'),
Expand All @@ -29,20 +31,23 @@ for (let i = 1; i < 100; i++) {
}

export default () => {
const dataGridRef = useRef();
const dataGridRef = useRef<EuiDataGridRefProps>(null);

// Modal
const [isModalVisible, setIsModalVisible] = useState(false);
const [lastFocusedCell, setLastFocusedCell] = useState({});
const [lastFocusedCell, setLastFocusedCell] = useState({
rowIndex: 0,
colIndex: 0,
});

const closeModal = useCallback(() => {
setIsModalVisible(false);
dataGridRef.current.setFocusedCell(lastFocusedCell); // Set the data grid focus back to the cell that opened the modal
dataGridRef.current!.setFocusedCell(lastFocusedCell); // Set the data grid focus back to the cell that opened the modal
}, [lastFocusedCell]);

const showModal = useCallback(({ rowIndex, colIndex }) => {
setIsModalVisible(true);
dataGridRef.current.closeCellPopover(); // Close any open cell popovers
dataGridRef.current!.closeCellPopover(); // Close any open cell popovers
setLastFocusedCell({ rowIndex, colIndex }); // Store the cell that opened this modal
}, []);

Expand Down Expand Up @@ -101,11 +106,18 @@ export default () => {

// Pagination
const [pagination, setPagination] = useState({ pageIndex: 0, pageSize: 25 });
const onChangePage = useCallback(
(pageIndex) =>
setPagination((pagination) => ({ ...pagination, pageIndex })),
[]
);
const onChangePage = useCallback((pageIndex) => {
setPagination((pagination) => ({ ...pagination, pageIndex }));
}, []);
const onChangePageSize = useCallback((pageSize) => {
setPagination((pagination) => ({ ...pagination, pageSize }));
}, []);

// Sorting
const [sortingColumns, setSortingColumns] = useState([]);
const onSort = useCallback((sortingColumns) => {
setSortingColumns(sortingColumns);
}, []);

// Manual cell focus
const [rowIndexAction, setRowIndexAction] = useState(0);
Expand All @@ -118,7 +130,7 @@ export default () => {
<EuiFormRow label="Row index">
<EuiFieldNumber
min={0}
max={24}
max={raw_data.length - 1}
value={rowIndexAction}
onChange={(e) => setRowIndexAction(Number(e.target.value))}
compressed
Expand All @@ -129,7 +141,7 @@ export default () => {
<EuiFormRow label="Column index">
<EuiFieldNumber
min={0}
max={4}
max={visibleColumns.length - 1}
value={colIndexAction}
onChange={(e) => setColIndexAction(Number(e.target.value))}
compressed
Expand All @@ -140,7 +152,7 @@ export default () => {
<EuiButton
size="s"
onClick={() =>
dataGridRef.current.setFocusedCell({
dataGridRef.current!.setFocusedCell({
rowIndex: rowIndexAction,
colIndex: colIndexAction,
})
Expand All @@ -153,7 +165,7 @@ export default () => {
<EuiButton
size="s"
onClick={() =>
dataGridRef.current.openCellPopover({
dataGridRef.current!.openCellPopover({
rowIndex: rowIndexAction,
colIndex: colIndexAction,
})
Expand All @@ -165,7 +177,7 @@ export default () => {
<EuiFlexItem grow={false}>
<EuiButton
size="s"
onClick={() => dataGridRef.current.setIsFullScreen(true)}
onClick={() => dataGridRef.current!.setIsFullScreen(true)}
>
Set grid to full screen
</EuiButton>
Expand All @@ -177,14 +189,17 @@ export default () => {
aria-label="Data grid demo"
columns={columns}
columnVisibility={{ visibleColumns, setVisibleColumns }}
sorting={{ columns: sortingColumns, onSort }}
inMemory={{ level: 'sorting' }}
rowCount={raw_data.length}
renderCellValue={({ rowIndex, columnId }) =>
raw_data[rowIndex][columnId]
}
pagination={{
...pagination,
pageSizeOptions: [25],
pageSizeOptions: [25, 50],
onChangePage: onChangePage,
onChangeItemsPerPage: onChangePageSize,
}}
height={400}
ref={dataGridRef}
Expand Down
6 changes: 3 additions & 3 deletions src/components/datagrid/body/data_grid_cell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ export class EuiDataGridCell extends Component<
rowManager,
...rest
} = this.props;
const { rowIndex, colIndex } = rest;
const { rowIndex, visibleRowIndex, colIndex } = rest;

const popoverIsOpen = this.isPopoverOpen();
const hasCellButtons = isExpandable || column?.cellActions;
Expand Down Expand Up @@ -534,7 +534,7 @@ export class EuiDataGridCell extends Component<
case keys.ENTER:
case keys.F2:
event.preventDefault();
openCellPopover({ rowIndex, colIndex });
openCellPopover({ rowIndex: visibleRowIndex, colIndex });
break;
}
} else {
Expand Down Expand Up @@ -641,7 +641,7 @@ export class EuiDataGridCell extends Component<
if (popoverIsOpen) {
closeCellPopover();
} else {
openCellPopover({ rowIndex, colIndex });
openCellPopover({ rowIndex: visibleRowIndex, colIndex });
}
}}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('EuiDataGridHeaderCell', () => {
<DataGridSortingContext.Provider
value={{
sorting: { ...sortingContext, ...sorting },
sortedRowMap: {},
sortedRowMap: [],
getCorrectRowIndex: jest.fn(),
}}
>
Expand Down
29 changes: 2 additions & 27 deletions src/components/datagrid/data_grid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
* Side Public License, v 1.
*/

import React, { useEffect, useState, createRef } from 'react';
import React, { useEffect, useState } from 'react';
import { mount, ReactWrapper, render } from 'enzyme';
import { EuiDataGrid } from './';
import { EuiDataGridProps, EuiDataGridRefProps } from './data_grid_types';
import { EuiDataGridProps } from './data_grid_types';
import {
findTestSubject,
requiredProps,
Expand Down Expand Up @@ -2749,29 +2749,4 @@ describe('EuiDataGrid', () => {
expect(takeMountedSnapshot(component)).toMatchSnapshot();
});
});

it('returns a ref which exposes internal imperative APIs', () => {
const gridRef = createRef<EuiDataGridRefProps>();

mount(
<EuiDataGrid
{...requiredProps}
columns={[{ id: 'A' }, { id: 'B' }]}
columnVisibility={{
visibleColumns: ['A', 'B'],
setVisibleColumns: () => {},
}}
rowCount={1}
renderCellValue={() => 'value'}
ref={gridRef}
/>
);

expect(gridRef.current).toEqual({
setIsFullScreen: expect.any(Function),
setFocusedCell: expect.any(Function),
openCellPopover: expect.any(Function),
closeCellPopover: expect.any(Function),
});
});
});
27 changes: 11 additions & 16 deletions src/components/datagrid/data_grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import React, {
useMemo,
useRef,
useState,
useImperativeHandle,
} from 'react';
import {
VariableSizeGrid as Grid,
Expand Down Expand Up @@ -56,6 +55,7 @@ import {
schemaDetectors as providedSchemaDetectors,
useMergedSchema,
} from './utils/data_grid_schema';
import { useImperativeGridRef } from './utils/ref';
import {
EuiDataGridColumn,
EuiDataGridProps,
Expand Down Expand Up @@ -302,23 +302,18 @@ export const EuiDataGrid = forwardRef<EuiDataGridRefProps, EuiDataGridProps>(
};

/**
* Expose internal APIs as ref to consumer
* Expose certain internal APIs as ref to consumer
*/
const { setFocusedCell } = focusContext; // eslint complains about the dependency array otherwise
const { openCellPopover, closeCellPopover } = cellPopoverContext;

useImperativeHandle(
useImperativeGridRef({
ref,
() => ({
setIsFullScreen,
setFocusedCell: ({ rowIndex, colIndex }) => {
setFocusedCell([colIndex, rowIndex]); // Transmog args from obj to array
},
openCellPopover,
closeCellPopover,
}),
[setFocusedCell, openCellPopover, closeCellPopover]
);
setIsFullScreen,
focusContext,
cellPopoverContext,
sortingContext,
pagination,
rowCount,
visibleColCount,
});

/**
* Classes
Expand Down
10 changes: 6 additions & 4 deletions src/components/datagrid/data_grid_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,13 @@ export interface EuiDataGridVisibleRows {

export interface DataGridSortingContextShape {
sorting?: EuiDataGridSorting;
sortedRowMap: { [key: number]: number };
getCorrectRowIndex: (rowIndex: number) => number;
sortedRowMap: number[];
getCorrectRowIndex: (visibleRowIndex: number) => number;
}

// An array of [x,y] coordinates. Note that the `y` value expected internally is a `visibleRowIndex`
export type EuiDataGridFocusedCell = [number, number];

export interface DataGridFocusContextShape {
focusedCell?: EuiDataGridFocusedCell;
setFocusedCell: (cell: EuiDataGridFocusedCell) => void;
Expand All @@ -196,6 +199,7 @@ export interface DataGridFocusContextShape {

export interface DataGridCellPopoverContextShape {
popoverIsOpen: boolean;
// Note that the rowIndex used to locate cells internally is a `visibleRowIndex`
cellLocation: { rowIndex: number; colIndex: number };
openCellPopover(args: { rowIndex: number; colIndex: number }): void;
closeCellPopover(): void;
Expand Down Expand Up @@ -764,8 +768,6 @@ export interface EuiDataGridInMemory {
skipColumns?: string[];
}

export type EuiDataGridFocusedCell = [number, number];

export interface EuiDataGridInMemoryValues {
[rowIndex: string]: { [columnId: string]: string };
}
Expand Down
Loading