Refactor/dropdown: Dropdown 컴포넌트 리팩토링

[prayinforrain]

🤠 개요

• Closes Refactor: Dropdown Props 변경 #68
• 이슈의 마지막 댓글에서 논의했던 대로, 이제 더 이상 Trigger Element를 실제 onClick Handler가 붙은 div로 감싸는 게 아니라 Element에 직접 바인딩해요. 접근성 태그도 마찬가지에요.
• Dropdown.List의 내용을 조건부 렌더링하지 않고 display:none;으로 숨겨요.
• 기존의 일관성 없던 id props를 필수 props label로 변경하고, 이를 기반으로 접근성 태그와 id를 붙이도록 했어요.

💫 설명

• display:none은 접근성 트리에서도 제거되기 때문에 따로 aria-hidden은 붙이지 않았어요.

• Context만을 반환하는 Dropdown에 id props가 있는게 혼동을 일으킨다고 생각해서 label로 이름을 변경했어요.

  • 이제 Trigger와 List 둘 다 label을 기반으로 id를 생성, 접근성 태그를 붙여요.

<Dropdown.Trigger> // Trigger div
  <button/> // 껍데기 element
</Dropdown.Trigger>

• 이슈에 남긴 토론이 길어서 간략하게 설명하자면, Trigger에는 button처럼 상호작용 가능한 요소가 올텐데 이를 실제 트리거 역할을 하는 div로 감싸는 것이 잘못 되었다 생각했어요.

  • 이는 키보드 접근성에서도 실제 Trigger div, 껍데기 element 두 개의 tabIndex가 생기는 부작용이 있어요.

  • 또 focus를 Trigger div에만 한정시켜야 하는 문제가 있어 css 스타일링에도 어려움이 있어요.

    • 예) Dropdown.Trigger 요소에 focus가 가기 때문에 Select Trigger의 정의된 형태에 :focus 스타일을 적용할 수 없었어요.

  • 그 외에 aria 태그가 껍데기 element가 아닌 그 위의 Trigger div에 붙어 있는 구조가 어색하다고 생각했어요.

• 결론: cloneElement가 Legacy API로 분류되어 있기는 하지만, Dropdown 담당자와 사용자 @se030의 논의 결과 현재 상황에서 최선의 선택이라고 생각했어요. 아직 deprecated 된 것도 아니고 props를 추가할 수 있는 대안이 없기 때문이에요. 더 좋은 방법이 생각나면 알려주세요!

📷 스크린샷 (Optional)

[se030]

저희 이런애들도 컨벤션 정해두고 맞춰서 써요 ㅎㅎ

[prayinforrain]

다음주 회의때 얘기해볼게요~!

[se030]

const Trigger = ({ children }: { children: ReactElement }) => {
  const context = useContext(DropdownContext);

  if (!context) return <></>;

  const { isOpen, setIsOpen, label, setTriggerSize } = context;

  useEffect(() => {
    const $trigger = document.getElementById(`${label}-Trigger`);
    if (!$trigger) return;
    setTriggerSize({
      width: $trigger.offsetWidth,
      height: $trigger.offsetHeight,
    });
  }, []);

  return cloneElement(children, {

이렇게 사용하면 가능할거에요! ReactNode는 아래처럼 생겼는데 Element 여러개를 받을 수도 있어서 타입 오류가 있었나봐요.

    type ReactFragment = Iterable<ReactNode>;
    type ReactNode = ReactElement | string | number | ReactFragment | ReactPortal | boolean | null | undefined;

// 이제 이건 되고
<Dropdown.Trigger>
  <Button text="팀원 목록" icon={MdPeople} variant="light" />
</Dropdown.Trigger>

// 이건 안돼요
<Dropdown.Trigger>
  <Button text="팀원 목록" icon={MdPeople} variant="light" />
  <Button text="팀원 목록" icon={MdPeople} variant="light" />
</Dropdown.Trigger>

[prayinforrain]

어!
어!!

죄송해요 제가 회의하던 내용을 뒤죽박죽 섞어서 잘못 반영했나봐요 헐
너무 창피해요 금방 고칠게요
헐..!

[prayinforrain]

이거 다시 원래 결론대로 수정했어요!

[se030]

  const onClick: MouseEventHandler = (e) => {
    e.stopPropagation();
    setIsOpen((prev) => !prev);
  };

  const onKeyDown: KeyboardEventHandler = (e) => {
    if (e.key === 'Enter') {
      setIsOpen((prev) => !prev);
    }
  };

핸들러도 이렇게 key랑 같은 네이밍으로 정의하고 넣어주면 더 깔끔할 것 같아요.
그리고 tabIndex, onKeyDown도 이번 작업에 넣어주실 수 있을까요? (요소가 tabbable이 아닌 경우도 있을 수 있어요)

[prayinforrain]

tabIndex는 반영할게요!
onKeyDown은 tabbable element인 경우 기본 동작 + 추가 핸들러가 같이 동작해서 열 수 없게 되지는 않나요?

[se030]

헉 열 수 없을 것 같네요 !! if 블럭 안에 e.preventDefault() 추가하면 되나요?

[prayinforrain]

스페이스바랑 엔터에 대해 작동하도록 반영했어요 😁

[se030]

id가 쏘아올린 작은공 ... 마감합니다 ...
너무너무너무너무 고생하셨어요 🎉🎉🎉 그래도 밥은 드세요 ....

[iyu88]

cloneElement 로 Trigger 의 children에 속성을 할당하는 건가요?
멋있는 방법같아요~