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

[Refactor] Dropdown 상태 관리 로직 리팩토링, UI 반영 #437

Merged
merged 8 commits into from
Jan 28, 2025

Conversation

wuzoo
Copy link
Contributor

@wuzoo wuzoo commented Jan 28, 2025

해당 이슈 번호

closed #436


체크리스트

  • 🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
  • 💯 테스트는 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 불필요한 코드는 제거했나요?
  • ✅ 컨벤션을 지켰나요?
  • 💭 이슈는 등록했나요?
  • 🏷️ 라벨은 등록했나요?
  • 🙇‍♂️ 리뷰어를 지정했나요?

💎 PR Point

Dropdown 컴포넌트를 수정해주었습니다. 수정을 하게 된 계기는 컴포넌트 설계에 대해서 다시 생각해볼 계기가 있었는데, 저희 드롭다운 컴포넌트가 이런 부분에서 수정이 필요하다고 느꼈어요 !

먼저 state를 관리하는 위치를 바꾸어주었습니다. 즉, isOpen 상태를 외부에서 관리할 필요 없이 Dropdown 컴포넌트 내부에서 자동으로 제어하도록 변경하였어요. 왜냐하면 Trigger 컴포넌트와 이에 대해 열고 닫히는 List 컴포넌트만이 open 상태를 알면 되기 때문에 DropdownRoot 컴포넌트에서 충분히 제어 가능한 상태라고 판단하였어요. 즉 의존성으로 주입할 필요가 없을 것 같다고 판단하였습니다.

DropdownTrigger 컴포넌트로 특정 요소를 감싸기만 한다면, 해당 요소를 클릭 시 list가 오픈되게 되요. 이는 트리거 컴포넌트에서 children API를 통해 자식 요소 클릭 이벤트에 overlay open 핸들러를 주입하고 있기 때문입니다. 드롭다운 합성 컴포넌트를 사용하는 사용처 컴포넌트에서 open 핸들러를 설정할 필요가 없게 됩니다.

트리거 컴포넌트는 드롭다운이 어떻게 열리냐에 따라 variant로 버튼과 인풋 두 개를 설정해주었어요. 버튼일 때는 클릭하면 드롭다운이 열리고, 인풋이라면 Focus 시 열리고 Blur 시 닫혀요. 이 때 Blur 이벤트가 리스트 아이템에 대한 Click 이벤트보다 우선순위가 높기 때문에 드롭다운의 아이템을 클릭하는 onSelect 핸들러가 실행되기 전에 blur로 인해 오버레이가 닫히면서 onSelect 콜백이 제대로 실행되지 않는 이슈가 발생했어요.

return (
    <li
      css={itemStyle}
      role="button"
      tabIndex={0}
      onKeyDown={handleKeyDown}
      onMouseDown={() => {
        onSelect?.();
        close();
      }}
      {...props}
    >
      {children}
    </li>
  );

따라서 이를 mousedown 이벤트로 변경해주었습니다. blur이벤트가 발생하기 전에 미리 클릭을 인지하고 이벤트를 발생시킬 수 있습니다.

Dropdown 컴포넌트를 수정해주었기 때문에 Select 컴포넌트 또한 불필요한 인터페이스를 삭제해주었습니다. 앞으로는 외부에서 overlay에 대한 불리언 값, 그리고 useOutsideClick 커스텀 훅을 통한 ref 주입 모두 안해주어도 자동으로 됩니다. 드롭다운 내부에서 모두 처리해두었어요 !


📌스크린샷 (선택)

2025-01-28.3.37.46.mov
2025-01-28.3.38.11.mov

Copy link

🚀 Storybook 확인하기 🚀

Copy link

🚀 Storybook 확인하기 🚀

Copy link
Member

@namdaeun namdaeun left a comment

Choose a reason for hiding this comment

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

확실히 드롭다운 내에서 context 써주니까 불필요한 코드들이 줄어든 것 같아서 좋네요
고생하셨습니다 ! 리액트 메서드들도 마니 배워가요 !!

{label && <Label id={label}>{label}</Label>}
{children}
</div>
<DropdownContext.Provider value={{ open, close, toggle, isOpen }}>
Copy link
Member

Choose a reason for hiding this comment

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

오 이렇게 ..!

packages/ui/src/Select/Select.tsx Outdated Show resolved Hide resolved
packages/ui/src/Select/Select.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@rtttr1 rtttr1 left a comment

Choose a reason for hiding this comment

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

오랜만에 보는 주용님 코드! 해당 pr 읽으면서 다시한번 컴포넌트를 설계할 때 책임과 관심사에 대해서 고민해봐야 한다는 사실을 상기시키고 갑니다!! 확실히 open 상태를 Dropdown 컴포넌트 내부에서 관리해 주니까 코드가 한결 깔끔해지네요~~ 고생하셨습니다~!

return (
<>
{Children.map(children, (child) => {
if (isValidElement(child)) {
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

@Bowoon1216 Bowoon1216 left a comment

Choose a reason for hiding this comment

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

설명과 코드를 읽는데도 어렵네요 🤯 역시~~ 우리리드님 LGTM

Copy link

🚀 Storybook 확인하기 🚀

@wuzoo wuzoo merged commit 05e1dfd into develop Jan 28, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI 컴포넌트 설계 개선
4 participants