Keyboard shortcut changes #1
Conversation
| - 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)) |
There was a problem hiding this comment.
Changelog entry was missing a link back to the PR
| @@ -1441,6 +1441,20 @@ Array [ | |||
|
|
|||
| describe('keyboard controls', () => { | |||
| it('supports simple arrow navigation', () => { | |||
| let pagination = { | |||
There was a problem hiding this comment.
Because of changes to data_grid.tsx, this test's pagination values/state needs to be tracked
| /> | ||
| ); | ||
|
|
||
| let focusableCell = getFocusableCell(component); | ||
| // focus should begin at the first cell |
There was a problem hiding this comment.
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
| const newPageRowCount = computeVisibleRows(props); | ||
| const pageCount = Math.ceil(rowCount / pageSize); | ||
| if (pageIndex < pageCount - 1) { | ||
| props.pagination.onChangePage(pageIndex + 1); |
There was a problem hiding this comment.
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 pageCount = Math.ceil(rowCount / pageSize); | ||
| if (pageIndex < pageCount - 1) { | ||
| props.pagination.onChangePage(pageIndex + 1); | ||
| const newPageRowCount = computeVisibleRows({ |
There was a problem hiding this comment.
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
| const rowIndex = | ||
| focusedCell[1] < newPageRowCount | ||
| ? focusedCell[1] | ||
| : newPageRowCount - 1; | ||
| setFocusedCell([focusedCell[0], rowIndex]); | ||
| requestAnimationFrame(() => updateFocus([focusedCell[0], rowIndex])); | ||
| updateFocus([focusedCell[0], rowIndex]); |
There was a problem hiding this comment.
The requestAnimationFrame has been moved into updateFocus itself
| setFocusedCell([focusedCell[0], focusedCell[1]]); | ||
| requestAnimationFrame(() => updateFocus([focusedCell[0], rowIndex])); | ||
| props.pagination.onChangePage(pageIndex - 1); | ||
| updateFocus(focusedCell); |
There was a problem hiding this comment.
When moving back/up through pages the focusedCell never changes as the row count will never decrease when paging backwards.
| const [cellsUpdateFocus, setCellsUpdateFocus] = useState< | ||
| Array<Function[] | null[]> | ||
| >([]); | ||
| const [cellsUpdateFocus] = useState<Map<string, Function>>(new Map()); |
There was a problem hiding this comment.
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.
|
|
||
| setCellsUpdateFocus(cellsUpdateFocus); | ||
| // this intentionally and purposefully mutates the existing `cellsUpdateFocus` object as the |
There was a problem hiding this comment.
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.
| @@ -163,30 +158,6 @@ export const EuiDataGridBody: FunctionComponent< | |||
| return rowMap; | |||
| }, [sorting, inMemory, inMemoryValues, schema, schemaDetectors]); | |||
|
|
|||
| const setCellFocus = useCallback( | |||
There was a problem hiding this comment.
This was meant to manage the distinction between focusedCell's value at the top data_grid.tsx (focusedCell is the column,row tuple where the values match the visible rows, but data_grid_row.tsx treated focusedCell's row on the row index instead. Row & Cell have now been provided the visible row index in addition to their logical row index, making this code block & its mapping obsolete.
| @@ -121,13 +122,13 @@ export class EuiDataGridCell extends Component< | |||
|
|
|||
| componentDidMount() { | |||
| this.unsubscribeCell = this.context.onFocusUpdate( | |||
| [this.props.colIndex, this.props.rowIndex], | |||
| [this.props.colIndex, this.props.visibleRowIndex], | |||
There was a problem hiding this comment.
All layers of datagrid now treat cell focus as the visible row index 🙌
[creating as a draft until I add inline comments]
This was much more straight forward and cleaner than I anticipated, nice job! This does not address the quickly-spamming-page-[up/down] and the focus issue, as I started digging into that I came across some bugs and performance things that I want to fix at the same time, which started affecting the scope of this PR - so I'm leaving that for a follow up task.