-
Notifications
You must be signed in to change notification settings - Fork 0
[URH-26] useHover 신규 #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/hooks/useHover.ts
Outdated
import { RefObject, useEffect, useRef, useState } from 'react'; | ||
|
||
const useHover = (): [RefObject<HTMLDivElement>, boolean] => { | ||
const targetRef = useRef<HTMLDivElement>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P5]
꼭 DivElement여야만 하나용?? HTMLElement 타입으로 설정해도 되지 않을까요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RefObject<HTMLDivElement>
로 정의한 이유는 두 가지가 있었는데요.
RefObject<HTMLElement>
로 정의했을 때 div 태그에 ref를 할당했더니 타입 에러가 발생했습니다.Property 'align' is missing in type 'HTMLElement' but required in type 'HTMLDivElement'.
- hover 이벤트를 수신할 요소와 target 요소를 분리하는 게 좋다고 생각했는데요. 예를 들어, target 컴포넌트에 hovered 스타일을 적용할 경우 해당 컴포넌트를 감싸는 요소를 추가하고 ref를 할당하는 것입니다.
이렇게 사용하는 게 더 안전하고 구조적으로 깔끔하다고 생각했고, 컴포넌트를 감싸는 건 div 태그로 해야 하기에
function Components() { const { ref, ...rest } = useHover(); return ( <div ref={ref}> <TargetComponent {...rest} /> </div> ) }
HTMLDivElement
로 ref 타입을 정의해 봤습니다.
추가로 덧붙여주실 의견 있으시면 주시면 감사하겠습니다!🙌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P4] HTMLDivElement
로 되어있으면 아래처럼 button 같은 인라인 요소 각각에 useHover를 적용하려면 div 태그를 추가하고 inline 스타일도 부여해야 하는 점이 있어서요..!
<div ref={ref} style={{ display: 'inline-block' }}>
<button>{isHovered ? 'Hovered!' : 'Not Hovered'}</button>
</div>
<div ref={ref2} style={{ display: 'inline-block' }}>
<button>{isHovered2 ? 'Hovered!' : 'Not Hovered'}</button>
</div>
사용자가 원하는 다양한 HTML 요소에도 useHover
를 적용할 수 있다면 더 좋을 것 같습니다!
아래처럼 제네릭을 사용하는 방법도 고려해봤는데, 훅을 사용할 때마다 타입을 명시적으로 지정해야 하는 점이 번거로울 수도 있을 것 같네요🤔
const useHover = <T extends HTMLElement>(): [RefObject<T>, boolean] => {
const targetRef = useRef<T>(null);
}
const [ref, isHovered] = useHover<HTMLDivElement>();
const [ref2, isHovered2] = useHover<HTMLButtonElement>();
return (
<>
<div ref={ref} style={{ background: isHovered ? '#f00' : '#fff' }}>
{isHovered ? 'Hovered!' : 'Not Hovered'}
</div>
<button ref={ref2}>{isHovered2 ? 'Hovered!' : 'Not Hovered'}</button>
</>
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 수현님처럼 제네릭을 사용해서 유저가 좀 더 유연하게 사용할 수 있도록 하면 좋겠다고 생각했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[commit] a94eb20
@Choozii @suhyeoonn 너무 좋은 방법입니다...👍🏻✨ 반영했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
src/hooks/useHover.ts
Outdated
const targetRef = useRef<HTMLDivElement>(null); | ||
const [isHovered, setIsHovered] = useState(false); | ||
|
||
const handleMouseEnter = () => setIsHovered(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P4] useEffect가 실행될 때마다 handleMouseEnter, handleMouseLeave가 생성될것 같은데, useCallback으로 감싸는건 어떠세용!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기론 말씀하신 함수들은 useHover가 실행될 때 한 번만 생성되고(리렌더링 시엔 재생성)
useEffect 내에선 실행만 되는 것으로 알고 있는데 혹시 제가 잘못 알고 있는 걸까요...?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 해당 컴포넌트가 다시 렌더링될때마다 생성되니 useCallback으로 최적화하는건 어떠냐는 제안이었습니다~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 지예 님 좋은 의견 감사드려요!
저는 useCallback(또는 useMemo)이 위 공식 문서에서 말하는 '어떠한 이익'을 확실하게 가져다주는 경우에만 사용하는데요.
해당 함수는 외부에 prop으로 공유되거나 의존하는 함수가 아니어서 useCallback으로 감싸도 아무런 이익이 없다고 생각해 사용하지 않았습니다!
혹시 제가 잘못 알고 있는 거라면 바로 잡아주시면 감사하겠습니다!🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
근데 isHovered가 해당 hook에서 상태값이라 isHovered 값이 변경될때마다 함수가 새로 정의될거 같아서요.
hover 같은 경우에는 유저가 사실 마우스를 움직이면서 isHovered 값이 많이 변경될수도 있는 인터랙션이라고 생각해서 useCallback으로 감싸는게 좋을것 같다구 생각했어요!ㅎㅎㅎㅎ
src/hooks/useHover.ts
Outdated
node.removeEventListener('mouseleave', handleMouseLeave); | ||
}; | ||
} | ||
}, [targetRef]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] targetRef가 변경될 가능성이 있는 상황을 생각하면 targetRef.current로 작성하는게 더 좋을것 같아요!
targetRef 자체는 리렌더링 돼도 계속 같은 객체를 참조하고 있어서 useEffect가 재실행이 안될거에유...!!!! (maybe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useHover는 ref를 한 번만 생성합니다(hovered를 수신해야 하는 ref가 여러개 필요하다면 useHover를 여러번 정의합니다).
그래서 targetRef 의존성 배열은 targetRef가 null인 상태와 null이 아닌 상태만을 구분하면 될 것 같아서 의도적으로 current를 선언하지 않았는데요.
혹시 이런 경우에도 current를 의존성에 포함시키는 게 이점이 있을까요?!🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useHover의 ref로 설정된 DOM 요소가 조건부 렌더링이 된다면 혹시 코드가 정상적으로 작동되나요? ref 값이 바뀌는 경우도 있지 않을까해서 말씀드렸습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지예 님 말씀대로 조건부 렌더링 시 useHover가 작동하지 않는 이슈가 있네요... 날카로운 지적 감사합니다!👍🏻👍🏻
그런데 조건부 렌더링 시엔 targetRef.current
를 의존성 배열에 넣어도 작동하지 않는 걸 확인했습니다. 이유는 렌더링 시에 ref를 참조하려는 시도를 했기 때문인데요.
이 문제를 RefCallback으로 해결할 수 있어서 해결해 봤습니다. 참고한 블로그 링크 첨부해 드리니 지예 님도 한 번 보셔도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bicochan 오왕 감사합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref관련해서 저도 고민점이 많아 리뷰를 남깁니다
현재까지 저도 커스텀훅을 작성하며 useRef의 ref 대신 setState를 return하여 지정하는 형식으로 관리해왔는데, useOutsideInteraction에서 지예님이 ref를 관리하는 법을 보고 고민의 과정에서 종찬님이 알려주신 useCallback 을 활용하는 방법까지 알게 되었는데 어떤 방식이 가장 적절할지 고민이 필요할 것 같습니다
또한, ref를 반환할때 각각 커스텀한 형태의 ref(예를 들어, intersectionRef, targetRef)를 줄것인가, 아니면 ref
로 통일할 것인가도 한번 정리해봐야할 거 같아요
각각 다른 ref이름를 지정한다면 일단 어떤 커스텀훅의 ref인가 구분이 쉽다는 장점이 있기는 하거든요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foresec ref의 적절한 사용은 항상 고민이 되는 부분인 것 같아요...🙃
추후에 이런 내용들을 Discussions에서 이야기해 보는 것도 좋을 것 같다는 생각이 드네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! 👍
👾 Pull Request
1️⃣ Spec
2️⃣ 변경 사항
3️⃣ 예시 코드