refactor: Reusable internal drag handle #3152
Conversation
pan-kot
force-pushed
the
internal-drag-handle
branch
from
b0e8100 to
35a4185
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3152 +/- ##
========================================
Coverage 96.40% 96.41%
========================================
Files 784 784
Lines 22136 22165 +29
Branches 7592 7203 -389
========================================
+ Hits 21341 21370 +29
Misses 788 788
Partials 7 7 ☔ View full report in Codecov by Sentry. |
pan-kot
force-pushed
the
internal-drag-handle
branch
from
35a4185 to
0f8c6c0
Compare
pan-kot
force-pushed
the
internal-drag-handle
branch
2 times, most recently
from
0a7d589 to
c5b4123
Compare
pan-kot
force-pushed
the
internal-drag-handle
branch
from
c5b4123 to
4f1bd4e
Compare
src/internal/events/index.ts
Outdated
| reactEvent: React.KeyboardEvent | ||
| ) { | ||
| if (!handler) { | ||
| return; |
There was a problem hiding this comment.
What's nice here, if the same check already happened on the line 71 anyway?
There was a problem hiding this comment.
true, but there are some occurrences of handler && fireKeyboardEvent(onKeyDown..) across the codebase. All other fire.*Event have this check built-in, so why not align this method too?
There was a problem hiding this comment.
Making it optional is nice. Duplicating already existing check is not
There was a problem hiding this comment.
Good catch - I will remove the unneeded if statement 👍
| ref: React.Ref<DragHandleProps.Ref> | ||
| ) => { | ||
| const dragHandleRefObject = useRef<HTMLElement>(null); | ||
| const dragHandleRef = useMergeRefs(dragHandleRefObject); |
There was a problem hiding this comment.
Good catch! There was the dragHandleRef originally but I had to replace it with the combination of the ref object and ref to use the useForwardFocus. While it is a common strategy in most components, the drag handle is still internal and does not feature the __internalRootRef - I will update it to only keep the ref object for now.
| @@ -9,7 +9,8 @@ export interface Focusable { | |||
| export interface FocusControlRefs { | |||
| toggle: RefObject<Focusable>; | |||
| close: RefObject<Focusable>; | |||
| slider: RefObject<HTMLDivElement>; | |||
| slider: RefObject<Focusable>; | |||
There was a problem hiding this comment.
why do we need to refs now? I see slider ref is used to focus on it inside Split panel. Can we instead use handle ref?
There was a problem hiding this comment.
I prepare the drag handle component to become a public component. Similar to other similar components, it does not give access to the dom node via ref but only allows to cast the focus programmatically - that is what the slider ref is used for.
The handle ref is a custom div wrapper around the drag handle to get the placement information from.
Description
Reusing drag handle between collection prefs, split panel, and drawers with potential future extension to board components, see: [a3UOAhaNX7KH].
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.