-
Notifications
You must be signed in to change notification settings - Fork 2
[#50] ✨ 공통 컴포넌트 dropdown 개발 #111
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
Conversation
|
혹시 프로필 눌렀을 때, 드롭다운 열리면서 프로필이 살짝 밀리는 건 스토리북 환경에서만 그런건가요 ?? |
yellowjang
left a comment
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.
작업량이 엄청나시네요 ...
윤호님 코드는 주석이 없어도 흐름대로 코드가 깔끔하게 잘 작성되어 있어서 가독성이 정말 뛰어난 것 같아요... 많이 배웁니다 ㅜㅜ
| }, []) | ||
|
|
||
| return ( | ||
| <DropdownContext.Provider value={{ isOpen, toggle, close }}> |
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.
Dropdown value에 상태를 넣어주는거군요..!
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 api 로 상태를 관리하고 있어요
| } | ||
|
|
||
| interface MenuProps extends BaseProps { | ||
| position?: 'dropup' | 'dropdown' |
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.
오 dropup/dropdown 으로 구분해서 position prop 나누어주는 거 좋은 것 같아요
| const Menu = ({ | ||
| children, | ||
| className = '', | ||
| position = 'dropdown', | ||
| alignment = 'left', | ||
| }: MenuProps): JSX.Element | null => { | ||
| const { isOpen } = useDropdownContext() | ||
| if (!isOpen) return null |
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.
이런 식으로 prop 넘겨주는 게 많다보면 사용할 때 복잡할 거라고 생각했었는데 오히려 어떤 것들을 지정해줘야 되는지 명확해서 좋은 것 같네용
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.
저도 바깥에서 classNames 를 내리느냐 vs 미리 props 를 연결해주느냐
관련해서 많은 고민을 했는데, 드롭다운을 쓰는 동료 개발자 입장에서는
처음부터 className을 임의로 주기에는 복잡도가 높은 것 같아서, 어느 정도 반복되는 패턴이 있으면 props 로 넘기고
예외가 있다면 바깥에서 추가로 스타일링을 입히는 방식으로 굳혔어요 ㅎㅎ
| const handleClick = (e: React.MouseEvent<HTMLElement>) => { | ||
| if (typeof onClick === 'function') { | ||
| onClick(e) | ||
| } |
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.
혹시 typeof onClick 가 function 일 경우에 onClick 이벤트가 발생하도록
처리하신 이유가 있으신가요 ??
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.
onClick 이 옵셔널 프롭스인데, onClick 이 undefined 일 때 호출을 하면 타입 에러가 발생해서,
onClick 을 받은 경우에만 호출하도록 타입 가드를 설정해줬어요
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.
아!! 그런 거였군요 ㅎㅎ 설명 감사합니다 :)
네 맞아요 저게 layout centered 라서, 자동으로 중간에 맞추려고 해서 그런데, |
제가 듣고 싶은 최고의 찬사네요 ㅎㅎ 감사합니다 |
📌 PR 템플릿
🏷️ PR 타입 (PR Type)
📝 요약 (Summary)
드롭다운 메뉴 또는 설렉트 컴포넌트의 재료가 되는 아토믹한 드롭다운의 개발
🔍 상세 내용 (Describe your changes)
🔗 관련 이슈 또는 링크 (Issue Number or Link)
✅ 체크리스트 (Checklist)
📸 스크린샷 (선택 사항)
2024-11-21.01.27.05.mov
📝 기타 사항