Refactor useRovingCellRef, fix test warnings #2883
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2883 +/- ##
==========================================
- Coverage 96.24% 96.22% -0.02%
==========================================
Files 38 38
Lines 1250 1246 -4
Branches 391 392 +1
==========================================
- Hits 1203 1199 -4
Misses 47 47
|
src/hooks/useRovingCellRef.ts
Outdated
| (cell: HTMLDivElement | null) => { | ||
| if (cell === null || !isSelected || cell.contains(document.activeElement)) return; | ||
|
|
||
| cell.focus({ preventScroll: true }); |
There was a problem hiding this comment.
My only concern is if it is possible to have a stale ref in a ref callback. Not sure why is useLayoutEffect is the recommended approach
There was a problem hiding this comment.
WDYM by stale ref?
useLayoutEffect would use that same ref anyway.
src/hooks/useRovingCellRef.ts
Outdated
|
|
||
| return { | ||
| ref, | ||
| ref: isSelected ? ref : undefined, | ||
| tabIndex: isFocused ? 0 : -1, | ||
| onFocus |
There was a problem hiding this comment.
onFocus: isSelected ? onFocus: undefined Would this work?
There was a problem hiding this comment.
It does, but then it does not catch programmatically focusing elements within a cell.
Then again we don't change the cell selection when we programmatically focus something within cells.
Extracted from #2459