Use mouse down event to select cell #3774
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3774 +/- ##
==========================================
- Coverage 98.79% 98.68% -0.11%
==========================================
Files 46 46
Lines 3475 3497 +22
Branches 762 763 +1
==========================================
+ Hits 3433 3451 +18
- Misses 42 46 +4
🚀 New features to boost your workflow:
|
| } | ||
| selectCellWrapper(); |
There was a problem hiding this comment.
onClick and onContextMenu events do not have any default behavior now but we are keeping the same signature as users may want to edit on single click or right click
| @@ -42,12 +42,14 @@ import type { | |||
| CalculatedColumn, | |||
| CellClickArgs, | |||
| CellClipboardEvent, | |||
| CellCopyEvent, | |||
| CellCopyArgs, | |||
There was a problem hiding this comment.
Renamed to match other types
src/DataGrid.tsx
Outdated
| const onCellPointerDownLatest = useLatestFunc(onCellPointerDown); | ||
| const onCellClickLatest = useLatestFunc(onCellClick); | ||
| const onCellDoubleClickLatest = useLatestFunc(onCellDoubleClick); |
There was a problem hiding this comment.
I wonder if we should remove useLatestFunc on these handlers and add a note on using useCallback in the readme. These events are probably not used often
| @@ -17,39 +17,41 @@ export { default as renderHeaderCell } from './renderHeaderCell'; | |||
| export { renderSortIcon, renderSortPriority } from './sortStatus'; | |||
| export { useRowSelection, useHeaderRowSelection } from './hooks'; | |||
| export type { | |||
| CalculatedColumn, | |||
| @@ -294,6 +294,9 @@ function HeaderFilters() { | |||
| rows={filteredRows} | |||
| headerRowHeight={filters.enabled ? 70 : undefined} | |||
| direction={direction} | |||
| style={{ | |||
| scrollbarGutter: 'stable' | |||
There was a problem hiding this comment.
Too much layout shift when filtering
| @@ -525,23 +533,17 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr | |||
| /** | |||
| * effects | |||
| */ | |||
| useLayoutEffect(() => { | |||
There was a problem hiding this comment.
We set shouldFocusCell so both effects were running when focus sink needs to be focused. I have combined them
There was a problem hiding this comment.
This effect didn't depend on shouldFocusCell, am I missing something?
There was a problem hiding this comment.
This effect was not checking shouldFocusCell but it was set to true and we were running both effects. I think it is better to only focus when shouldFocusCell flag is true so there are no surprises
There was a problem hiding this comment.
Was it never running when shouldFocusCell was false?
There was a problem hiding this comment.
I don't believe so
| @@ -525,23 +533,17 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr | |||
| /** | |||
| * effects | |||
| */ | |||
| useLayoutEffect(() => { | |||
There was a problem hiding this comment.
This effect didn't depend on shouldFocusCell, am I missing something?
| @@ -37,7 +37,7 @@ function SummaryCell<R, SR>({ | |||
| typeof summaryCellClass === 'function' ? summaryCellClass(row) : summaryCellClass | |||
| ); | |||
|
|
|||
| function onClick() { | |||
| function onPointerDown() { | |||
| selectCell({ rowIdx, idx: column.idx }); | |||
There was a problem hiding this comment.
I wonder if we should have unified event handlers at the DataGrid level instead of having the same implementations in various components.
Something like
function onCellEvent(event: any event, pointerArgs: which row/cell) {
// if pointer down...
// if click...
}There was a problem hiding this comment.
I was thinking the same when I added #3774 (comment). I will check
There was a problem hiding this comment.
I tried but it is getting little complex with group row and summary row event handlers. I will revisit
Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
…data-grid into am-cell-selection
amanmahajan7
changed the title
I checked a few popular grids and all select the cell on pointer down but they do not scroll the cell into view which we do. Not sure which behavior is better
🆕
onCellMouseDownevent