[feat] Select 컴포넌트 생성 #45
Conversation
✅ Deploy Preview for thejulge1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
almighty55555
requested review from
ghdtnals
and removed request for
jeonghwanJay
almighty55555
requested review from
jeonghwanJay
and removed request for
ghdtnals
cozy-ito
left a comment
cozy-ito
left a comment
There was a problem hiding this comment.
고생 많으셨습니다! 👍
몇 가지 코드 리뷰 적어보았습니다만,
하나의 의견이라고 생각해주시면 감사하겠습니다 😄
There was a problem hiding this comment.
Select 컴포넌트의 리스트가 나타나고 난 뒤, 같은 가로 열 어딘가를 클릭했을 때 닫히지 않는 버그가 있습니다.
There was a problem hiding this comment.
그리고 버튼이나 리스트에 호버했을 때, 마우스 커서가 pointer로 변경되면 좋을 것 같아요! 🤔
src/components/Select.tsx
Outdated
| const wrapperRef = useRef<HTMLDivElement>(null); | ||
| const buttonRef = useRef<HTMLButtonElement>(null); | ||
|
|
||
| const selectedOption = options.find((option) => option.value === value); |
There was a problem hiding this comment.
어느 정도 크기의 options prop이 전달될 지 모르니
useMemo로 최적화 해봐도 좋을 것 같아요! 😄
There was a problem hiding this comment.
현재 사용하는 옵션 수는 기껏해야 20개 정도라서 성능 이슈가 없다고 판단했습니다!
불필요한 deps 배열 관리로 인한 가독성 저하를 막는 것이 현재로서는 최적화보다 우선이라는 판단 하에 useMemo를 사용하지 않고 작성하였는데 .. 혹시 다른 의견이 있으시다면 더 제안해주시면 감사하겠습니다!
There was a problem hiding this comment.
가독성 부분은 selectedOption이라는 변수명에서 어떤 역할을 하는지에 대해서 잘 나타내고 있다고 저는 생각을 했습니다 하핳.
때문에 현재는 서울권 지역만 해당되어 20가지이지만,
본래는 전역권의 지역일테니 미리 최적화 코드를 적용하는 것이 어떨까 했습니다 😅
적용 여부는 조금 더 고민해 봐 주시고 자유롭게 결정해주셔도 좋을 것 같아요! 👍
There was a problem hiding this comment.
흠... 맞는 말입니다ㅎㅎ 피그마에 구현된 수준에서만 고민했던 것 같네요 그럼 적용해보겠습니다!~
| const listClassNames = cn( | ||
| "absolute top-full left-0 mt-1 border rounded-[0.375rem] bg-white border-gray-30 shadow-lg z-10 max-h-48 overflow-y-auto", | ||
| ); |
There was a problem hiding this comment.
| const listClassNames = cn( | |
| "absolute top-full left-0 mt-1 border rounded-[0.375rem] bg-white border-gray-30 shadow-lg z-10 max-h-48 overflow-y-auto", | |
| ); | |
| const listClassNames = cn( | |
| "absolute top-full left-0 mt-1 border rounded-[0.375rem] bg-white border-gray-30 text-black shadow-lg z-10 max-h-48 overflow-y-auto", | |
| ); |
리스트 아이템에 text-black 스타일이 적용되어 있지 않아요.
현재는 #000000이지만, 수민님께서 등록해주신 테마 색상인 text-black은 엄연히 #111322로 다르니까요! 👍
텍스트와 관련된 속성은 대부분 하위 자식요소까지 상속되기 때문에 리스트에 클래스명을 주면 좋을 것 같습니다! 😄
| const buttonClassNames = cn( | ||
| "flex items-center justify-between rounded-[0.375rem]", | ||
| { | ||
| "w-full": fullWidth, | ||
| "bg-white placeholder:text-gray-40 border border-gray-30": size === "lg", | ||
| "bg-gray-10 font-bold": size === "sm", | ||
| }, | ||
| sizeMap[size], | ||
| className, | ||
| ); |
There was a problem hiding this comment.
span 태그를 사용하지 않고, button 태그의 className속성으로만 placeholder의 스타일을 제어하는 건 어떨까요? 🤔
그리고 value가 존재한다면, text-black이 적용 되어야 합니다. 😅
| const buttonClassNames = cn( | |
| "flex items-center justify-between rounded-[0.375rem]", | |
| { | |
| "w-full": fullWidth, | |
| "bg-white placeholder:text-gray-40 border border-gray-30": size === "lg", | |
| "bg-gray-10 font-bold": size === "sm", | |
| }, | |
| sizeMap[size], | |
| className, | |
| ); | |
| const buttonClassNames = cn( | |
| "flex items-center justify-between rounded-[0.375rem]", | |
| { | |
| "w-full": fullWidth, | |
| "bg-white placeholder:text-gray-40 border border-gray-30": size === "lg", | |
| "bg-gray-10 font-bold": size === "sm", | |
| }, | |
| value ? "text-black" : "text-gray-40", // 추가 | |
| sizeMap[size], | |
| className, | |
| ); |
<button
id={id}
type="button"
ref={buttonRef}
onClick={() => setOpen((prev) => !prev)}
className={buttonClassNames}
style={width ? { width } : undefined}
{...rest}
>
{/* <span className={cn(value ? "" : "text-gray-40")}> */}
{selectedOption?.label || placeholder}
{/* </span> */}
{open ? <DropdownUp className="ml-2" /> : <DropdownDown className="ml-2" />}
</button>
src/components/Select.tsx
Outdated
| fullWidth, | ||
| width, |
There was a problem hiding this comment.
width, fullWidth 속성이 없어도 className으로 제어 할 수 있을 것 같아요!
리스트의 가로 너비도 useEffect로 동기화 시켜주고 있으니까요 하하 😄
There was a problem hiding this comment.
fullWidth 같은 경우 TextField 컴포넌트와 통일성을 맞추기 위해 유지하는 게 더 좋을 것 같습니당. width는 말씀하신 것처럼 className으로 제어하는 게 더 좋아보이네요! 리뷰 내용 반영해 PR 올리겠습니당
jeonghwanJay
left a comment
jeonghwanJay
left a comment
There was a problem hiding this comment.
고생하셨습니다 !
만약 추후에 Select 사용 시 이슈가 발생하면 공유 드리겠습니다.
#️⃣연관된 이슈
📝 PR 유형
📝작업 내용
Select컴포넌트 구현 완료했습니다. 자세한 구현 내용은 아래 위키 문서 참고 부탁드립니다!Select 위키
작업 과정 중 변동사항
Button컴포넌트에도cn을 적용했습니다.index.css파일에 스크롤바 설정을 등록했습니다.Select컴포넌트를 생성했습니다.Select위키 문서를 작성했습니다.스크린샷 (선택)
💬리뷰 요구사항(선택)