-
Notifications
You must be signed in to change notification settings - Fork 1
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: <Picker> 구현 #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 고생하셨습니다!! 리뷰가 쪼끔 늦었죠 호호... 코드도 읽어보고 스토리북으로 이것저것 해보는데 다 잘돼서 넘우 신기했습니다 이런 걸 다 고려하다니 증말 세심하군요 현스...... 궁금한 점이랑 수정 필요한 부분 코멘트 달아놨어요!!!
import { PickerProps } from './Picker.type'; | ||
import { StyledPicker } from './Picker.style'; | ||
|
||
export const Picker = forwardRef<HTMLDivElement, PickerProps>(({ children, ...props }, ref) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picker 한 개당 최대 3개의 PickerColumn을 가질 수 있다는 제한 사항이 있어서 갯수 제한을 설정해주시면 더 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 디자인적으로만 고려해도 충분할 것 같고, 추후 확장 가능성을 위해 의도적으로 추가하지 않았어요.
갯수 제한을 걸려면 context로 컴포넌트의 수를 센 다음에 3개 초과면 오류를 던저야 하는데, 이 정도의 오버헤드를 감수하면서 개발 단에서 제한을 추가할 필요는 없을 것 같습니다.
1️⃣ 어떤 작업을 했나요? (Summary)
기존 코드에 영향을 미치지 않는 변경사항
useDOMRef
훅 추가useUncontrolledValue()
훅 추가<Picker>
컴포넌트 추가2️⃣ 알아두시면 좋아요!
<Picker>
컴포넌트의 구현이 조금 복잡하게 되어 있어요. 주석의 개선이나 코드를 조금 더 깔끔하게 할 수 있는 방법이 있다면 알려주세요!3️⃣ 추후 작업
4️⃣ 체크리스트 (Checklist)
main
브랜치의 최신 코드를pull
받았나요?