[DataGrid] Rework keyboard navigation #3193
Conversation
flaviendelangle
added
core
There was a problem hiding this comment.
Much more simple to understand. 👍
On v6 I think we should replace the cellNavigationKeyDown and columnHeaderNavigationKeyDown with apiRef methods.
Yeah, it's weird to have a keyboard event that is published only when a specific set of keys are pressed but even when these keys are pressed it's ignored, e.g. on cell editing. This is one reason I made it private in #1572. I think cellKeyDown should be used for everything.
packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts
Outdated
Show resolved
Hide resolved
| if (colIdx === 0) { | ||
| newRowIndex = currentPage.range.firstRowIndex; | ||
| const viewportPageSize = apiRef.current.unstable_getViewportPageSize(); | ||
| const colIndexBefore = params.field ? apiRef.current.getColumnIndex(params.field) : 0; |
There was a problem hiding this comment.
Because there is one case where this event is triggered after comiting a row edit and we don't have a field.
It was working because getColumnIndex returned -1 and the clamp behavior set it back to 0, but it's not great calling it that way.
I was one of the inconsistencies I found while typing the event system.
There was a problem hiding this comment.
Makes sense. The event below shouldn't be published.
Goals
Remove abstractions whenever possible to make the code easier to read
viewportPageSize/firstRowIndexInPage/lastRowIndexInPage/firstColIndex/lastColIndexto make the switch statement as clear as possiblegoToCellandgoToHeadermethod to abstract the scroll and focus behaviorsFollow up
On v6 I think we should replace the
cellNavigationKeyDownandcolumnHeaderNavigationKeyDownwithapiRefmethods.For me events should be "hey I did X, do whatever you want with this information" not "hey, could someone listen to me and do X".