Skip to content
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

feat(react): useFocus 신규 훅 추가 #510

Merged
merged 13 commits into from
Oct 16, 2024

Conversation

99mini
Copy link
Contributor

@99mini 99mini commented Oct 16, 2024

Overview

Issue: #352

ref가 할당된 element가 focus 상태인지를 반환하고, 강제로 포커스할 수 있는 함수를 제공합니다.

  • ref를 등록하지 않거나, 액션(focusAction, blurAction)을 등록하지 않았을 경우 에러를 던지지 않고, 아무런 동작하지 않습니다.

Usage

const { ref, isFocus, setFocus } = useFocus<HTMLInputElement>({
  focusAction: () => console.log("focus"),
  blurAction: () => console.log("blur"),
});

return (
  <div>
    <input ref={ref} />
    <button onClick={() => setFocus()}>focus trigger</button>
    <div>{isFocus ? 'Focus' : 'Blur'}</div>
  </div>
)

PR Checklist

  • All tests pass.
  • All type checks pass.
  • I have read the Contributing Guide document.
    Contributing Guide

@99mini 99mini requested a review from ssi02014 as a code owner October 16, 2024 03:33
Copy link

changeset-bot bot commented Oct 16, 2024

🦋 Changeset detected

Latest commit: 5abebda

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modern-kit/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ssi02014 ssi02014 added feature 새로운 기능 추가 @modern-kit/react @modern-kit/react labels Oct 16, 2024
@ssi02014 ssi02014 linked an issue Oct 16, 2024 that may be closed by this pull request
2 tasks
Comment on lines 42 to 77
export function useFocus<T extends HTMLElement>({
focusAction = noop,
blurAction = noop,
}: UseFocusProps = {}): UseFocusReturnType<T> {
const [isFocus, setIsFocus] = useState(false);

const ref = useRef<T>(null);

const onFocus = useCallback(
(event: FocusEvent) => {
setIsFocus(true);
focusAction(event);
},
[focusAction]
);

const onBlur = useCallback(
(event: FocusEvent) => {
setIsFocus(false);
blurAction(event);
},
[blurAction]
);

const setFocus = useCallback(() => {
if (ref.current) {
ref.current.focus();
setIsFocus(true);
}
}, []);

useEventListener(ref, 'focus', onFocus);
useEventListener(ref, 'blur', onBlur);

return { ref, isFocus, setFocus };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

제가 잘못 안내드린 부분이라 죄송스러운 부분인데요. 인자로 받는 focusAction, blurAction의 각각의 네이밍을 onFocus, onBlur로 변경하고자 합니다. 이는 useHover의 네이밍 on{Action}과 통일이 목적입니다.

  • useHover 등 내부 함수 네이밍을 통일 계획이 있습니다. (ex. preserved{~~})

기존 onFoucs, onBlur는 useCallback이 아닌 참조를 유지해주는 usePreservedCallback로 대체합니다. 결국 인자로 넘겨주는 focusAction, blurAction은 함수 즉 참조형이기 떄문에 리액트가 리렌더링 시 마다 다른 함수로 판단하고, 내부 onFocus, onBlur도 useCallback이지만 새로운 함수를 생성합니다.

즉, 아래와 같이 최종 제안을 드려봅니다.

import { noop } from '@modern-kit/utils';
import { useEventListener } from '../useEventListener';
import { usePreservedCallback } from '../usePreservedCallback';
import { RefObject, useCallback, useRef, useState } from 'react';

interface UseFocusProps {
  onFocus?: (event: FocusEvent) => void;
  onBlur?: (event: FocusEvent) => void;
}

interface UseFocusReturnType<T extends HTMLElement> {
  ref: RefObject<T>;
  isFocus: boolean;
  setFocus: () => void;
}

export function useFocus<T extends HTMLElement>({
  onFocus = noop,
  onBlur = noop,
}: UseFocusProps = {}): UseFocusReturnType<T> {
  const [isFocus, setIsFocus] = useState(false);

  const ref = useRef<T>(null);

  const preservedFocusAction = usePreservedCallback((event: FocusEvent) => {
    setIsFocus(true);
    onFocus(event);
  });

  const preservedBlurAction = usePreservedCallback((event: FocusEvent) => {
    setIsFocus(false);
    onBlur(event);
  });

  const setFocus = useCallback(() => {
    if (!ref.current) return;

    ref.current.focus();
    setIsFocus(true);
  }, []);

  useEventListener(ref, 'focus', preservedFocusAction);
  useEventListener(ref, 'blur', preservedBlurAction);

  return { ref, isFocus, setFocus };
}

Copy link
Contributor

Choose a reason for hiding this comment

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

문서도 추가로 변경되어야 합니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

빠르고 상세한 리뷰 감사합니다!!

기존 onFoucs, onBlur는 useCallback이 아닌 참조를 유지해주는 usePreservedCallback로 대체합니다. 결국 인자로 넘겨주는 focusAction, blurAction은 함수 즉 참조형이기 떄문에 리액트가 리렌더링 시 마다 다른 함수로 판단하고, 내부 onFocus, onBlur도 useCallback이지만 새로운 함수를 생성합니다.

이미 구현된 usePreservedCallback 함수 사용 안내 감사합니다.

구현, 테스트, 문서에 각각 변경된 인터페이스 작용하였습니다!

Comment on lines 46 to 48
await act(async () => {
await user.click(targetTrigger);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 코드에 포함된 await act는 모두 제거되어도 됩니다 🤗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그렇군요 감사합니다!

expect(blurMockFn).toBeCalled();
});

it('should not throw error when ref is not connected', async () => {
Copy link
Contributor

@ssi02014 ssi02014 Oct 16, 2024

Choose a reason for hiding this comment

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

Suggested change
it('should not throw error when ref is not connected', async () => {
it('should not perform any actions when ref is not assigned', async () => {

테스트 설명에 에러를 던지지 않는 것에 포커스를 두기보다 아무런 동작을 하지 않는다에 포커스를 두는게 의도에 적절하다고 생각합니다 :)

expect(blurMockFn).not.toBeCalled();
});

it('should not throw error when focusAction and blurAction is not provided', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should not throw error when focusAction and blurAction is not provided', async () => {
it('should not perform any actions when onFocus and onBlur are not assigned', async () => {

Copy link
Contributor

@ssi02014 ssi02014 left a comment

Choose a reason for hiding this comment

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

modern-kit에대한 관심과 기여에 정말 감사드립니다 🤗

@ssi02014 ssi02014 merged commit 6561a60 into modern-agile-team:main Oct 16, 2024
2 checks passed
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
@99mini 99mini deleted the feature/useFocus branch October 16, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 새로운 기능 추가 @modern-kit/react @modern-kit/react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: useFocus
2 participants