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] Provide row elements to wrap cells #5285

Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
3de3ce9
Provide rows for datagrid cells to be owned by
chandlerprall Sep 23, 2021
4594779
Merge branch 'master' into bug/4474-datagrid-row-accessibility
chandlerprall Sep 27, 2021
04d3ab5
changelog
chandlerprall Sep 27, 2021
9a6dc07
Merge branch 'master' into bug/4474-datagrid-row-accessibility
chandlerprall Oct 5, 2021
f786f5f
undoing things
chandlerprall Oct 5, 2021
06ac499
working except virtualized container
chandlerprall Oct 6, 2021
0129ea4
working row wrapper implementation
chandlerprall Oct 7, 2021
c05b457
Create datagrid row elements on-demand and render cells via portals
chandlerprall Oct 19, 2021
179da4a
Provide rows for datagrid cells to be owned by
chandlerprall Sep 23, 2021
24f5c53
changelog
chandlerprall Sep 27, 2021
a2ccb72
undoing things
chandlerprall Oct 5, 2021
eb4344c
working except virtualized container
chandlerprall Oct 6, 2021
535a2a9
working row wrapper implementation
chandlerprall Oct 7, 2021
9151350
Create datagrid row elements on-demand and render cells via portals
chandlerprall Oct 19, 2021
814feb1
Small style cleanup
chandlerprall Oct 19, 2021
95b7008
updated changelog
chandlerprall Oct 19, 2021
0ba3a02
updated changelog
chandlerprall Oct 19, 2021
1569aa0
fixing a bad changelog merge
chandlerprall Oct 19, 2021
9a6df52
oh that style was important
chandlerprall Oct 19, 2021
ae336f9
Update src/components/datagrid/body/data_grid_row_manager.ts
chandlerprall Oct 26, 2021
8dc8ad4
Update src/components/datagrid/data_grid_types.ts
chandlerprall Oct 26, 2021
7633c1b
Ensure unique ID in combobox with prepended or appended labels (#5229)
1Copenut Oct 19, 2021
c3e8b49
update i18ntokens
chandlerprall Oct 19, 2021
5ac481f
40.0.0
chandlerprall Oct 19, 2021
e0dd86e
Updated documentation.
chandlerprall Oct 19, 2021
ca721f2
Add combined Jest+Cypress code coverage reports (#5262)
constancecchen Oct 20, 2021
86af5ba
[EuiMarkdownEditor] Add `remark-breaks` and line break plugin (#5272)
i-a-n Oct 20, 2021
2553d89
[Docs] Separated out Borders to its own page & [EuiTableRowCell] fixe…
cchaos Oct 20, 2021
fdc387e
Fix CL from #5272
cchaos Oct 25, 2021
d468c85
[Docs] Update EuiDatePicker types (#5318)
thompsongl Oct 25, 2021
dea1eb9
[Cypress] Add flakey test retries + harden intermittent context menu …
constancecchen Oct 25, 2021
c995387
[Docs] Fix Colors guidelines (#5316)
thompsongl Oct 26, 2021
a60d734
Update dependency @elastic/charts to ^38.1.0 (#5321)
renovate[bot] Oct 26, 2021
ae515b6
PR feedback
chandlerprall Oct 26, 2021
f5b02ef
Added a cypress test for datagrid row rendering
chandlerprall Nov 1, 2021
82614cf
Merge branch 'master' into bug/4474-datagrid-row-accessibility-alternate
chandlerprall Nov 1, 2021
8336957
Merge branch 'master' into bug/4474-datagrid-row-accessibility-alternate
chandlerprall Nov 1, 2021
baa7b25
Revert changes(?) to docs/
chandlerprall Nov 1, 2021
937403e
revert changelog reformats
chandlerprall Nov 1, 2021
705edad
re-ignore a couple datagrid example pages when running a11y tests
chandlerprall Nov 2, 2021
55a5f95
Alternate way to know if the datagrid cells have rendered
chandlerprall Nov 3, 2021
420b37d
Merge branch 'main' into bug/4474-datagrid-row-accessibility-alternate
chandlerprall Nov 4, 2021
ad5c72b
PR feedback
chandlerprall Nov 4, 2021
6d10f54
changelog
chandlerprall Nov 5, 2021
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
- Updated `tokenKeyword` to match the definition of keyword field type ([#5251](https://github.com/elastic/eui/pull/5251))
- Added `element`, `buttonElement`, and `arrowProps` props to further customize `EuiAccordion` ([#5258](https://github.com/elastic/eui/pull/5258))

**Bug fixes**

- Fixed an accessibility issue where `EuiDataGrid` cells weren't owned by `role=row` elements ([#5213](https://github.com/elastic/eui/pull/5285))
cee-chen marked this conversation as resolved.
Show resolved Hide resolved

**Breaking changes**

- Renamed `tokenKeyword` icon to `tokenTag` in `EuiToken` ([#5251](https://github.com/elastic/eui/pull/5251))
Expand Down
9 changes: 0 additions & 9 deletions scripts/a11y-testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,6 @@ const docsPages = async (root, page) => {
`${root}#/forms/color-selection`,
`${root}#/forms/date-picker`,
`${root}#/forms/super-date-picker`,
`${root}#/tabular-content/data-grid`,
`${root}#/tabular-content/data-grid-in-memory-settings`,
`${root}#/tabular-content/data-grid-schemas-and-popovers`,
`${root}#/tabular-content/data-grid-focus`,
`${root}#/tabular-content/data-grid-styling-and-control`,
`${root}#/tabular-content/data-grid-control-columns`,
`${root}#/tabular-content/data-grid-footer-row`,
`${root}#/tabular-content/data-grid-virtualization`,
`${root}#/tabular-content/data-grid-row-heights-options`,
];

return [
Expand Down
1 change: 1 addition & 0 deletions src/components/datagrid/_data_grid_data_row.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.euiDataGridRow {
// needed for footer cells to correctly position
display: flex;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ exports[`EuiDataGridCell renders 1`] = `
<EuiDataGridCell
colIndex={0}
columnId="someColumn"
id="cell"
interactiveCellId="someId"
isExpandable={true}
popoverContent={[Function]}
Expand Down Expand Up @@ -53,6 +54,7 @@ exports[`EuiDataGridCell renders 1`] = `
<Memo()
colIndex={0}
columnId="someColumn"
id="cell"
isDetails={false}
isExpandable={true}
isExpanded={false}
Expand Down Expand Up @@ -86,6 +88,7 @@ exports[`EuiDataGridCell renders 1`] = `
<renderCellValue
columnId="someColumn"
data-test-subj="cell-content"
id="cell"
isDetails={false}
isExpandable={true}
isExpanded={false}
Expand Down
13 changes: 13 additions & 0 deletions src/components/datagrid/body/data_grid_body.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@ import {
import {
EuiDataGridBodyProps,
EuiDataGridInMemoryValues,
EuiDataGridRowManager,
EuiDataGridSchemaDetector,
} from '../data_grid_types';
import { makeRowManager } from './data_grid_row_manager';

export const VIRTUALIZED_CONTAINER_CLASS = 'euiDataGrid__virtualized';

Expand All @@ -72,6 +74,7 @@ export const Cell: FunctionComponent<GridChildComponentProps> = ({
rowHeightsOptions,
getRowHeight,
rowHeightUtils,
rowManager,
} = data;

const { headerRowHeight } = useContext(DataGridWrapperRowsContext);
Expand Down Expand Up @@ -139,6 +142,7 @@ export const Cell: FunctionComponent<GridChildComponentProps> = ({
getRowHeight={getRowHeight}
rowHeightsOptions={rowHeightsOptions}
rowHeightUtils={rowHeightUtils}
rowManager={rowManager}
style={{
...style,
top: `${parseFloat(style.top as string) + headerRowHeight}px`,
Expand Down Expand Up @@ -166,6 +170,7 @@ export const Cell: FunctionComponent<GridChildComponentProps> = ({
rowHeightsOptions={rowHeightsOptions}
getRowHeight={getRowHeight}
rowHeightUtils={rowHeightUtils}
rowManager={rowManager}
style={{
...style,
top: `${parseFloat(style.top as string) + headerRowHeight}px`,
Expand Down Expand Up @@ -203,6 +208,7 @@ export const Cell: FunctionComponent<GridChildComponentProps> = ({
rowHeightsOptions={rowHeightsOptions}
getRowHeight={getRowHeight}
rowHeightUtils={rowHeightUtils}
rowManager={rowManager}
style={{
...style,
top: `${parseFloat(style.top as string) + headerRowHeight}px`,
Expand Down Expand Up @@ -616,6 +622,11 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
const wrapperRef = useRef<HTMLDivElement | null>(null);
const wrapperDimensions = useResizeObserver(wrapperRef.current);

const innerGridRef = useRef<HTMLDivElement | null>(null);
const [rowManager] = useState<EuiDataGridRowManager>(() =>
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
makeRowManager(innerGridRef)
);

// reset height constraint when rowCount changes
useEffect(() => {
setHeight(wrapperRef.current!.getBoundingClientRect().height);
Expand Down Expand Up @@ -694,6 +705,7 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
{...(virtualizationOptions ? virtualizationOptions : {})}
ref={setGridRef}
innerElementType={InnerElement}
innerRef={innerGridRef}
className={VIRTUALIZED_CONTAINER_CLASS}
columnCount={
leadingControlColumns.length +
Expand Down Expand Up @@ -724,6 +736,7 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
interactiveCellId,
rowHeightsOptions,
rowHeightUtils,
rowManager,
}}
rowCount={
IS_JEST_ENVIRONMENT || headerRowHeight > 0
Expand Down
1 change: 1 addition & 0 deletions src/components/datagrid/body/data_grid_cell.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { EuiDataGridCell } from './data_grid_cell';

describe('EuiDataGridCell', () => {
const requiredProps = {
id: 'cell',
Copy link
Member

Choose a reason for hiding this comment

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

[not a blocking comment, just me asking out of curiosity] Is the id prop actually required? I'm not seeing that change as part of this PR, unless I'm missing something

rowIndex: 0,
visibleRowIndex: 0,
colIndex: 0,
Expand Down
13 changes: 11 additions & 2 deletions src/components/datagrid/body/data_grid_cell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import React, {
memo,
MutableRefObject,
} from 'react';
import { createPortal } from 'react-dom';
import tabbable from 'tabbable';
import { keys } from '../../../services';
import { EuiScreenReaderOnly } from '../../accessibility';
Expand All @@ -33,6 +34,7 @@ import {
} from '../data_grid_types';
import { EuiDataGridCellButtons } from './data_grid_cell_buttons';
import { EuiDataGridCellPopover } from './data_grid_cell_popover';
import { IS_JEST_ENVIRONMENT } from '../../../test';

const EuiDataGridCellContent: FunctionComponent<
EuiDataGridCellValueProps & {
Expand Down Expand Up @@ -145,6 +147,7 @@ export class EuiDataGridCell extends Component<
};

setCellRef = (ref: HTMLDivElement | null) => {
// save the reference
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
this.cellRef.current = ref;

// watch the first cell for size changes and use that to re-compute row heights
Expand Down Expand Up @@ -404,9 +407,11 @@ export class EuiDataGridCell extends Component<
className,
column,
style,
rowHeightsOptions,
rowManager,
...rest
} = this.props;
const { rowIndex, rowHeightsOptions } = rest;
const { rowIndex } = rest;

const showCellButtons =
this.state.isFocused ||
Expand Down Expand Up @@ -595,7 +600,7 @@ export class EuiDataGridCell extends Component<
}
}

return (
const content = (
<div
role="gridcell"
tabIndex={
Expand All @@ -617,5 +622,9 @@ export class EuiDataGridCell extends Component<
{innerContent}
</div>
);

return rowManager && !IS_JEST_ENVIRONMENT
? createPortal(content, rowManager.getRow(rowIndex))
: content;
}
}
47 changes: 47 additions & 0 deletions src/components/datagrid/body/data_grid_row_manager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { RefObject } from 'react';
import { EuiDataGridRowManager } from '../data_grid_types';

export const makeRowManager = (
containerRef: RefObject<HTMLDivElement>
): EuiDataGridRowManager => {
const rowIdToElements = new Map<number, HTMLDivElement>();

return {
reset() {
rowIdToElements.clear();
},
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
getRow(rowId) {
let rowElement = rowIdToElements.get(rowId);
Copy link
Member

Choose a reason for hiding this comment

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

Small naming nit: I'd suggest changing this to arg to rowIndex over rowId for clarity. When I think of IDs I think of strings rather than numbers.

Suggested change
getRow(rowId) {
let rowElement = rowIdToElements.get(rowId);
getRow(rowIndex) {
let rowElement = rowIdToElements.get(rowIndex);


if (rowElement == null) {
rowElement = document.createElement('div');
rowElement.setAttribute('role', 'row');
chandlerprall marked this conversation as resolved.
Show resolved Hide resolved
rowElement.classList.add('euiDataGridRow');
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
rowIdToElements.set(rowId, rowElement);
chandlerprall marked this conversation as resolved.
Show resolved Hide resolved

// add the element to the wrapping container
containerRef.current?.appendChild(rowElement);

// watch the row's children, if they all disappear then remove this row
const observer = new MutationObserver((records) => {
if ((records[0].target as HTMLElement).childElementCount === 0) {
observer.disconnect();
rowElement?.remove();
rowIdToElements.delete(rowId);
}
});
observer.observe(rowElement, { childList: true });
}

return rowElement;
},
};
};
4 changes: 1 addition & 3 deletions src/components/datagrid/data_grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ import {
EuiDataGridStyleRowHover,
} from './data_grid_types';
import { RowHeightUtils } from './row_height_utils';

// Used to short-circuit some async browser behaviour that is difficult to account for in tests
const IS_JEST_ENVIRONMENT = global.hasOwnProperty('_isJest');
import { IS_JEST_ENVIRONMENT } from '../../test';

// Each gridStyle object above sets a specific CSS select to .euiGrid
const fontSizesToClassMap: { [size in EuiDataGridStyleFontSizes]: string } = {
Expand Down
8 changes: 7 additions & 1 deletion src/components/datagrid/data_grid_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ export interface EuiDataGridCellProps {
style?: React.CSSProperties;
rowHeightsOptions?: EuiDataGridRowHeightsOptions;
rowHeightUtils?: RowHeightUtils;
rowManager?: EuiDataGridRowManager;
}

export interface EuiDataGridCellState {
Expand All @@ -396,7 +397,7 @@ export interface EuiDataGridCellState {

export type EuiDataGridCellValueProps = Omit<
EuiDataGridCellProps,
'width' | 'interactiveCellId' | 'popoverContent'
'width' | 'interactiveCellId' | 'popoverContent' | 'rowManager'
>;
export interface EuiDataGridControlColumn {
/**
Expand Down Expand Up @@ -724,3 +725,8 @@ export interface EuiDataGridRowHeightsOptions {
*/
lineHeight?: string;
}

export interface EuiDataGridRowManager {
reset(): void;
getRow(rowId: number): HTMLDivElement;
chandlerprall marked this conversation as resolved.
Show resolved Hide resolved
}
1 change: 1 addition & 0 deletions src/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ export {
stopThrowingReactWarnings,
} from './react_warnings';
export { sleep } from './sleep';
export { IS_JEST_ENVIRONMENT } from './is_jest';
9 changes: 9 additions & 0 deletions src/test/is_jest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export const IS_JEST_ENVIRONMENT = global.hasOwnProperty('_isJest');