feat: add ref support to Scanner component #143
Conversation
|
@yudielcurbelo Please review this PR! |
There was a problem hiding this comment.
Pull Request Overview
Adds ref support to the Scanner component by forwarding a ref to the underlying video element and introducing a mergeRefs utility to combine internal and external refs.
- Convert Scanner to use React.forwardRef and forward the ref to the
- Introduce utilities/mergeRefs to merge the forwarded ref with the internal videoRef
- Minor formatting and displayName assignment
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/utilities/mergeRefs.ts | New helper to merge multiple refs; used to combine forwarded ref with internal ref. |
| src/components/Scanner.tsx | Refactor Scanner to forwardRef, wire merged ref to the |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,18 @@ | |||
| import { RefCallback, RefObject } from 'react'; | |||
|
|
|||
| export function mergeRefs<T>(...refs: Array<RefObject<T> | RefCallback<T> | null | undefined>): RefCallback<T> { | |||
There was a problem hiding this comment.
The signature excludes MutableRefObject and uses RefObject, which makes passing useRef refs (e.g., MutableRefObject<HTMLVideoElement | null>) a type error. Switch to React.Ref (or include MutableRefObject<T | null>) so both object and callback refs are accepted.
Copilot uses AI. Check for mistakes.
| continue; | ||
| } | ||
|
|
||
| (ref as RefObject<T | null>).current = value; |
There was a problem hiding this comment.
RefObject has a readonly current; assigning to .current here is a type error. Cast to MutableRefObject<T | null> (or narrow with an appropriate type guard) before assignment: (ref as MutableRefObject<T | null>).current = value.
Copilot uses AI. Check for mistakes.
| }} | ||
| className={classNames?.video} autoPlay muted playsInline /> | ||
| <video | ||
| ref={mergeRefs(ref, videoRef)} |
There was a problem hiding this comment.
With the current mergeRefs signature (Array<RefObject | RefCallback ...>), videoRef (MutableRefObject<HTMLVideoElement | null>) is not assignable, producing a type error. This will be resolved by updating mergeRefs to accept React.Ref (or include MutableRefObject<T | null>) as suggested.
Copilot uses AI. Check for mistakes.
1 participant
resolved #140