Skip to content

[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

Merged
merged 3 commits into from
Jul 30, 2024
Merged

[URH-26] useHover 신규 #29

merged 3 commits into from
Jul 30, 2024

Conversation

bicochan
Copy link
Member

👾 Pull Request

1️⃣ Spec

  • target 요소에 할당할 ref와 hovered 여부를 반환합니다.

2️⃣ 변경 사항

  • 없음

3️⃣ 예시 코드

const Component = () => {
  const [ref, isHovered] = useHover();

  return (
    <div ref={ref} style={{ background: isHovered ? '#f00' : '#fff' }}>
      {isHovered ? 'Hovered!' : 'Not Hovered'}
    </div>
  );
};

@Choozii Choozii self-requested a review July 25, 2024 00:26
import { RefObject, useEffect, useRef, useState } from 'react';

const useHover = (): [RefObject<HTMLDivElement>, boolean] => {
const targetRef = useRef<HTMLDivElement>(null);
Copy link
Member

@Choozii Choozii Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P5]
꼭 DivElement여야만 하나용?? HTMLElement 타입으로 설정해도 되지 않을까요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RefObject<HTMLDivElement>로 정의한 이유는 두 가지가 있었는데요.

  1. RefObject<HTMLElement>로 정의했을 때 div 태그에 ref를 할당했더니 타입 에러가 발생했습니다.
    Property 'align' is missing in type 'HTMLElement' but required in type 'HTMLDivElement'.
    
  2. hover 이벤트를 수신할 요소와 target 요소를 분리하는 게 좋다고 생각했는데요. 예를 들어, target 컴포넌트에 hovered 스타일을 적용할 경우 해당 컴포넌트를 감싸는 요소를 추가하고 ref를 할당하는 것입니다.
    function Components() {
      const { ref, ...rest } = useHover();
      return (
        <div ref={ref}>
          <TargetComponent {...rest} />
        </div>
      )
    }
    이렇게 사용하는 게 더 안전하고 구조적으로 깔끔하다고 생각했고, 컴포넌트를 감싸는 건 div 태그로 해야 하기에 HTMLDivElement로 ref 타입을 정의해 봤습니다.

추가로 덧붙여주실 의견 있으시면 주시면 감사하겠습니다!🙌🏻

Copy link
Contributor

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>
  </>
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 수현님처럼 제네릭을 사용해서 유저가 좀 더 유연하게 사용할 수 있도록 하면 좋겠다고 생각했습니다!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[commit] a94eb20

@Choozii @suhyeoonn 너무 좋은 방법입니다...👍🏻✨ 반영했습니다!

Copy link
Member

@Choozii Choozii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

const targetRef = useRef<HTMLDivElement>(null);
const [isHovered, setIsHovered] = useState(false);

const handleMouseEnter = () => setIsHovered(true);
Copy link
Member

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으로 감싸는건 어떠세용!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 알기론 말씀하신 함수들은 useHover가 실행될 때 한 번만 생성되고(리렌더링 시엔 재생성)
useEffect 내에선 실행만 되는 것으로 알고 있는데 혹시 제가 잘못 알고 있는 걸까요...?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아아 해당 컴포넌트가 다시 렌더링될때마다 생성되니 useCallback으로 최적화하는건 어떠냐는 제안이었습니다~!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 지예 님 좋은 의견 감사드려요!

저는 useCallback(또는 useMemo)이 위 공식 문서에서 말하는 '어떠한 이익'을 확실하게 가져다주는 경우에만 사용하는데요.

해당 함수는 외부에 prop으로 공유되거나 의존하는 함수가 아니어서 useCallback으로 감싸도 아무런 이익이 없다고 생각해 사용하지 않았습니다!

혹시 제가 잘못 알고 있는 거라면 바로 잡아주시면 감사하겠습니다!🙏🏻

Copy link
Member

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으로 감싸는게 좋을것 같다구 생각했어요!ㅎㅎㅎㅎ

node.removeEventListener('mouseleave', handleMouseLeave);
};
}
}, [targetRef]);
Copy link
Member

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)

Copy link
Member Author

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를 의존성에 포함시키는 게 이점이 있을까요?!🧐

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useHover의 ref로 설정된 DOM 요소가 조건부 렌더링이 된다면 혹시 코드가 정상적으로 작동되나요? ref 값이 바뀌는 경우도 있지 않을까해서 말씀드렸습니다!

Copy link
Member Author

@bicochan bicochan Jul 29, 2024

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으로 해결할 수 있어서 해결해 봤습니다. 참고한 블로그 링크 첨부해 드리니 지예 님도 한 번 보셔도 좋을 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bicochan 오왕 감사합니다!

Copy link
Collaborator

@foresec foresec Jul 29, 2024

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인가 구분이 쉽다는 장점이 있기는 하거든요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@foresec ref의 적절한 사용은 항상 고민이 되는 부분인 것 같아요...🙃

추후에 이런 내용들을 Discussions에서 이야기해 보는 것도 좋을 것 같다는 생각이 드네요!

@bicochan bicochan requested a review from suhyeoonn July 28, 2024 15:57
Copy link
Contributor

@suhyeoonn suhyeoonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다! 👍

@bicochan bicochan merged commit 9b857a1 into master Jul 30, 2024
6 checks passed
@bicochan bicochan deleted the URH-26/use-hover branch July 30, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants