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.

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

Keyboard shortcut changes #1

Merged
merged 3 commits into from
Nov 20, 2019
Merged

Keyboard shortcut changes #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c01784
EuiDataGrid keyboard navigation tweaks
chandlerprall Nov 19, 2019
05c7f10
Refactored cellsUpdateFocus usage
chandlerprall Nov 19, 2019
4cbb9fa
commented on an anti-pattern
chandlerprall Nov 20, 2019
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
## [`master`](https://github.com/elastic/eui/tree/master)

- Added new keyboard shortcuts for the data grid component: `Home` (same row, first column), `End` (same row, last column), `Ctrl+Home` (first row, first column), `Ctrl+End` (last row, last column), `Page Up` (next page) and `Page Down` (previous page)
- Added `badge` prop and new styles `EuiHeaderAlert` ([#2506](https://github.com/elastic/eui/pull/2506))
- Added new keyboard shortcuts for the data grid component: `Home` (same row, first column), `End` (same row, last column), `Ctrl+Home` (first row, first column), `Ctrl+End` (last row, last column), `Page Up` (next page) and `Page Down` (previous page) ([#2519](https://github.com/elastic/eui/pull/2519))
Copy link
Author

Choose a reason for hiding this comment

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

Changelog entry was missing a link back to the PR


## [`16.0.1`](https://github.com/elastic/eui/tree/v16.0.1)

Expand Down
88 changes: 52 additions & 36 deletions src/components/datagrid/data_grid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,6 +1441,20 @@ Array [

describe('keyboard controls', () => {
it('supports simple arrow navigation', () => {
let pagination = {
Copy link
Author

Choose a reason for hiding this comment

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

Because of changes to data_grid.tsx, this test's pagination values/state needs to be tracked

pageIndex: 0,
pageSize: 3,
pageSizeOptions: [3, 6, 10],
onChangePage: (pageIndex: number) => {
pagination = {
...pagination,
pageIndex,
};
component.setProps({ pagination });
},
onChangeItemsPerPage: () => {},
};

const component = mount(
<EuiDataGrid
{...requiredProps}
Expand All @@ -1453,151 +1467,157 @@ Array [
renderCellValue={({ rowIndex, columnId }) =>
`${rowIndex}, ${columnId}`
}
pagination={{
pageIndex: 0,
pageSize: 3,
pageSizeOptions: [3, 6, 10],
onChangePage: () => {},
onChangeItemsPerPage: () => {},
}}
pagination={pagination}
/>
);

let focusableCell = getFocusableCell(component);
// focus should begin at the first cell
Copy link
Author

@chandlerprall chandlerprall Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The chain of updates/expects grew to a point where it wasn't obvious what each block was doing/testing, so I added comments to each to help me follow

expect(focusableCell.length).toEqual(1);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A');

// focus should not move when up against the left edge
focusableCell
.simulate('focus')
.simulate('keydown', { keyCode: keyCodes.LEFT });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A'); // focus should not move when up against an edge
).toEqual('0, A');

// focus should not move when up against the top edge
focusableCell.simulate('keydown', { keyCode: keyCodes.UP });
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A'); // focus should not move when up against an edge
).toEqual('0, A');

// move down
focusableCell.simulate('keydown', { keyCode: keyCodes.DOWN });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('1, A');

// move right
focusableCell.simulate('keydown', { keyCode: keyCodes.RIGHT });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('1, B');

// move up
focusableCell.simulate('keydown', { keyCode: keyCodes.UP });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, B');

// move left
focusableCell.simulate('keydown', { keyCode: keyCodes.LEFT });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A');

// move down and to the end of the row
focusableCell
.simulate('keydown', { keyCode: keyCodes.DOWN })
.simulate('keydown', { keyCode: keyCodes.END });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('1, C');

// move up and to the beginning of the row
focusableCell
.simulate('keydown', { keyCode: keyCodes.UP })
.simulate('keydown', { keyCode: keyCodes.HOME });

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A');

// jump to the last cell
focusableCell.simulate('keydown', {
ctrlKey: true,
keyCode: keyCodes.END,
});

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('2, C');

// jump to the first cell
focusableCell.simulate('keydown', {
ctrlKey: true,
keyCode: keyCodes.HOME,
});

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A');

// page should not change when moving before the first entry
focusableCell.simulate('keydown', {
keyCode: keyCodes.PAGE_UP,
}); // 0, A

});
focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A'); // focus should not move when up against an edge
).toEqual('0, A');

// advance to the next page
focusableCell.simulate('keydown', {
keyCode: keyCodes.PAGE_DOWN,
}); // 3, A

});
focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('3, A');

// move over one column and advance one more page
focusableCell
.simulate('keydown', { keyCode: keyCodes.RIGHT }) // 3, B
.simulate('keydown', {
keyCode: keyCodes.PAGE_DOWN,
}); // 6, B
focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('6, B');

// does not advance beyond the last page
focusableCell.simulate('keydown', {
keyCode: keyCodes.PAGE_DOWN,
});
focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('6, B'); // should move page forward and keep focus on the same cell
).toEqual('6, B');

// move left one column, return to the previous page
focusableCell
.simulate('keydown', { keyCode: keyCodes.LEFT }) // 6, A
.simulate('keydown', {
keyCode: keyCodes.PAGE_UP,
}); // 3, A

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('3, A'); // back one page, at the first cell
).toEqual('3, A');

// return to the previous (first) page
focusableCell.simulate('keydown', {
keyCode: keyCodes.PAGE_UP,
}); // 0, A

});
focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A'); // should be back in the first page
).toEqual('0, A');

// move to the last cell of the page then advance one page
focusableCell
.simulate('keydown', {
ctrlKey: true,
Expand All @@ -1606,24 +1626,21 @@ Array [
.simulate('keydown', {
keyCode: keyCodes.PAGE_DOWN,
}); // 5, C (last cell of the second page, same cell position as previous page)

focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('5, C');

// advance to the final page, but there is 1 row less on page 3 so focus should retreat a row but retain the column
focusableCell.simulate('keydown', {
keyCode: keyCodes.PAGE_DOWN,
}); // 7, C (should recalculate row since there is not as many rows as previous page)

}); // 7, C
focusableCell = getFocusableCell(component);
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('7, C');
// (equivalent cell position does not exist in last page (would be 8, C),
// so keeps the same column position. but moves to the last available row,
// which should be (7, C))
});

it('does not break arrow key focus control behavior when also using a mouse', () => {
const component = mount(
<EuiDataGrid
Expand All @@ -1641,7 +1658,6 @@ Array [
);

let focusableCell = getFocusableCell(component);
// console.log(focusableCell.debug());
expect(
focusableCell.find('[data-test-subj="cell-content"]').text()
).toEqual('0, A');
Expand Down
66 changes: 29 additions & 37 deletions src/components/datagrid/data_grid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ const cellPaddingsToClassMap: {
l: 'euiDataGrid--paddingLarge',
};

function computeVisibleRows(props: EuiDataGridProps) {
function computeVisibleRows(
props: Pick<EuiDataGridProps, 'pagination' | 'rowCount'>
) {
const { pagination, rowCount } = props;

const startRow = pagination ? pagination.pageIndex * pagination.pageSize : 0;
Expand Down Expand Up @@ -320,36 +322,34 @@ function createKeyDownHandler(
} else if (keyCode === keyCodes.PAGE_DOWN) {
if (props.pagination) {
event.preventDefault();
const totalRowCount = props.rowCount;
const rowCount = props.rowCount;
const pageIndex = props.pagination.pageIndex;
const pageSize = props.pagination.pageSize;
const pageCount = Math.ceil(totalRowCount / pageSize);
if (pageIndex < pageCount) {
props.pagination!.pageIndex = pageIndex + 1;
props.pagination.onChangePage(props.pagination.pageIndex);
const newPageRowCount = computeVisibleRows(props);
const pageCount = Math.ceil(rowCount / pageSize);
if (pageIndex < pageCount - 1) {
props.pagination.onChangePage(pageIndex + 1);
Copy link
Author

Choose a reason for hiding this comment

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

This (and PAGE_UP) modified props.pagination directly, which leads to unexpected behavior. This changes to use the onChangePage callback which matches how other pagination changes occur. (this is the change that caused the test case to track pagination state)

const newPageRowCount = computeVisibleRows({
Copy link
Author

Choose a reason for hiding this comment

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

I refactored computeVisibleRows to only take specifically rowCount and pagination, allowing those to be more customized when passing into that function. This was necessary due to no longer directly modifying props

rowCount,
pagination: {
...props.pagination,
pageIndex: pageIndex + 1,
},
});
const rowIndex =
focusedCell[1] < newPageRowCount
? focusedCell[1]
: newPageRowCount - 1;
setFocusedCell([focusedCell[0], rowIndex]);
requestAnimationFrame(() => updateFocus([focusedCell[0], rowIndex]));
updateFocus([focusedCell[0], rowIndex]);
Copy link
Author

Choose a reason for hiding this comment

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

The requestAnimationFrame has been moved into updateFocus itself

}
}
} else if (keyCode === keyCodes.PAGE_UP) {
if (props.pagination) {
event.preventDefault();
const pageIndex = props.pagination.pageIndex;
if (pageIndex > 0) {
props.pagination!.pageIndex = pageIndex - 1;
props.pagination.onChangePage(props.pagination.pageIndex);
const newPageRowCount = computeVisibleRows(props);
const rowIndex =
focusedCell[1] < newPageRowCount
? focusedCell[1]
: newPageRowCount - 1;
setFocusedCell([focusedCell[0], focusedCell[1]]);
requestAnimationFrame(() => updateFocus([focusedCell[0], rowIndex]));
props.pagination.onChangePage(pageIndex - 1);
updateFocus(focusedCell);
Copy link
Author

Choose a reason for hiding this comment

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

When moving back/up through pages the focusedCell never changes as the row count will never decrease when paging backwards.

}
}
} else if (keyCode === (ctrlKey && keyCodes.END)) {
Expand Down Expand Up @@ -598,37 +598,29 @@ export const EuiDataGrid: FunctionComponent<EuiDataGridProps> = props => {
</EuiI18n>
);

const [cellsUpdateFocus, setCellsUpdateFocus] = useState<
Array<Function[] | null[]>
>([]);
const [cellsUpdateFocus] = useState<Map<string, Function>>(new Map());
Copy link
Author

Choose a reason for hiding this comment

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

Instead of the nested arrays to track onFocusUpdate listeners, I refactored to be a map. I think this results in more readable code - I quickly get lost when working with nested arrays and all those [ ] brackets.


const updateFocus = (focusedCell: [number, number]) => {
const updateFocus = cellsUpdateFocus[focusedCell[0]][focusedCell[1]];

if (updateFocus) {
updateFocus();
const key = `${focusedCell[0]}-${focusedCell[1]}`;
if (cellsUpdateFocus.has(key)) {
requestAnimationFrame(() => {
cellsUpdateFocus.get(key)!();
});
}
};

const datagridContext = {
onFocusUpdate: (cell: [number, number], updateFocus: Function) => {
if (pagination) {
// Receives the row index as for the whole set
// and normalizes it for the visible rows in the grid
const pageIndex = pagination.pageIndex;
const pageSize = pagination.pageSize;
const rowIndex = Math.ceil(cell[1] - pageIndex * pageSize);

if (!cellsUpdateFocus[cell[0]]) {
cellsUpdateFocus[cell[0]] = [];
}

cellsUpdateFocus[cell[0]][rowIndex] = updateFocus;
const key = `${cell[0]}-${cell[1]}`;

setCellsUpdateFocus(cellsUpdateFocus);
// this intentionally and purposefully mutates the existing `cellsUpdateFocus` object as the
Copy link
Author

Choose a reason for hiding this comment

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

To be honest I don't know of The React Way to manage this interaction. I'll think more about it, but this is clean enough to merge in. There's a similar situation in the datagrid's in-memory rendering & tracking of cell values. Likely, we need to find a better solution for all of EUI than abusing requestAnimationFrame in these circumstances.

// value/state of `cellsUpdateFocus` must be up-to-date when `updateFocus`'s requestAnimationFrame fires
// there is likely a better pattern to use, but this is fine for now as the scope is known & limited
cellsUpdateFocus.set(key, updateFocus);

return () => {
cellsUpdateFocus[cell[0]][rowIndex] = null;
cellsUpdateFocus.delete(key);
};
}
},
Expand Down
Loading