[Refactor] DropDownOption 에 대해서 context 로 분리 #93
Conversation
… into refactor/dropdown-context
SeieunYoo
requested review from
ghdtjgus76,
eugene028,
hamo-o and
kongnayeon
as code owners
| handleSelect(value); | ||
| }; | ||
|
|
||
| const setOptionRef = (index: number) => (el: HTMLDivElement | null) => { | ||
| optionsRef.current[index] = el; | ||
| }; | ||
|
|
||
| const renderTrigger = (trigger: ReactElement) => ( | ||
| const renderCustomTrigger = (trigger: ReactElement) => ( | ||
| <> | ||
| {cloneElement(trigger, { | ||
| onClick: toggleDropdown, | ||
| "aria-expanded": open, | ||
| "aria-haspopup": "true", | ||
| id: `${dropdownId}-trigger`, | ||
| "aria-controls": `${dropdownId}`, | ||
| })} | ||
| </> | ||
| ); |
There was a problem hiding this comment.
p3;
renderCustomTrigger에서도 cloneElement가 아니라 context api 방식으로 해결해볼 수 있을 거 같다는 생각이 드는데, 별도의 컴포넌트로 분리 후 context api 적용하도록 개선해보면 좋지 않을까 싶습니다!
There was a problem hiding this comment.
context api 를 적용하려면 trigger 외부에 div 같은 엘리먼트로 하나 감싸서 프로퍼티를 전달해주어야 하는데 요렇게 되면 의도하지 않은 외부 div 가 돔에 생겨서 스타일링에 영향을 줄 수도 있을 것 같은데 어떻게 생각하시나용?!
There was a problem hiding this comment.
음 그럴 수도 있겠네요!
당장은 best practice가 뭔지 잘 모르겠어서 조금 더 고민해봐야 할 거 같아요!
추후에 더 좋은 방법이 생각난다면 개선해봐도 좋을 거 같은...
|
DropDown 리뷰 반영하면서 코드들 찾아보다 드롭다운 옵션 컴포넌트의 전체 value 관리 관련해서 좋은 해결 방법을 찾아서 추가로 리팩토링 해두었어요! 요 방식은 value 를 관리하는 Collection 이라는 context 를 추가로 만드는 방법입니다. Option 컴포넌트에서 collection 관련 context 를 불러와서 value 값을 업데이트해주는 방식입니다. 그래서 기존에 children 을 flat 해서 value 와 text 를 관리하는 로직을 삭제했고 useDropDownState 에서는 selectedValue,open,focusedValue 만 관리하도록 수정해두었습니다! 나머지 코드 리뷰들은 이 리팩토링을 통해 해결되었어요!! +) 추가적으로 export 되서는 안되지만 내부 개발에서 사용되는 컴포넌트들이 생겨서 빌드 스크립트에서 제외해주기 위해 임시방편으로 f8527b2 컴포넌트 네임을 통해 제외시켜주도록 했습니다!! cli 로 얼른 만들어야겠어요.. 🙏 |
… into refactor/dropdown-context
| const updateFocusedValue = useCallback( | ||
| (direction: number) => { | ||
| const values = Array.from(itemMap.keys()); | ||
| setFocusedValue((prevValue) => { | ||
| const currentIndex = values.indexOf(prevValue ?? ""); | ||
| const nextIndex = | ||
| (currentIndex + direction + values.length) % values.length; | ||
| return values[nextIndex] ?? ""; | ||
| }); | ||
| }, | ||
| [itemMap, setFocusedValue] | ||
| ); | ||
|
|
||
| const handleKeyDown = useCallback( | ||
| (event: KeyboardEvent<HTMLDivElement>) => { | ||
| if (!open) return; | ||
|
|
||
| const { key } = event; | ||
|
|
||
| if (key === "ArrowDown") { | ||
| updateFocusedValue(1); | ||
| event.preventDefault(); | ||
| } else if (key === "ArrowUp") { | ||
| updateFocusedValue(-1); | ||
| event.preventDefault(); | ||
| } else if (key === "Enter" && focusedValue !== null) { | ||
| handleSelect(focusedValue, itemMap.get(focusedValue)?.text); | ||
| event.preventDefault(); | ||
| } | ||
| }, | ||
| [open, focusedValue, updateFocusedValue, handleSelect, itemMap] | ||
| ); |
There was a problem hiding this comment.
p4;
<Flex
direction="column"
role="listbox"
style={{ visibility: open ? "visible" : "hidden" }}
visibility={open ? "visible" : "hidden"}
className={dropdownContentStyle({
type: trigger ? "custom" : "default",
})}
>
{children}
</Flex>
현재 DropDown 컴포넌트 내부에 있는 Flex 컴포넌트와 children을 DropDownOptionList로 만들고 focus 관련 로직을 그 하위에서 관리해주면 어떨까 합니다!
뭔가 focus 관련 로직은 Wrapper 쪽이 아니라 OptionList 쪽에서 관리해주는 것이 이해하기 쉬울 거 같아요
DropDownWrapper에서는 정말 wrapper의 역할만 하는 느낌으로 변경하면 조금 더 깔끔하지 않을까 하는,,
There was a problem hiding this comment.
좋아요!! DropDownOptionList 컴포넌트에서 focus 처리하도록 수정해두었어요 6ca0434
| type ContextValue = { | ||
| itemMap: Map<ItemData["value"], { text: ItemData["text"] }>; | ||
| }; |
There was a problem hiding this comment.
p3;
혹시 text 부분은 객체로 사용한 이유가 있을까요?
객체로 나타내지 않고 string으로도 나타내볼 수 있을 거 같다는 생각이 듭니다!
There was a problem hiding this comment.
처음에는 ref 랑 text 같이 전달해서 구현하다가 text 만 넘기는 걸로 수정돼서 객체로 구현할 필요가 없을 것 같네요!! 6440016 요기서 text 만 넘기도록 수정했습니다
🎉 변경 사항
🚩 관련 이슈
🙏 여기는 꼭 봐주세요!