-
Notifications
You must be signed in to change notification settings - Fork 4
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
[FE] 달력 로직 개선 #727
base: develop
Are you sure you want to change the base?
[FE] 달력 로직 개선 #727
Conversation
객체에서 배열로 변경
dataLoading을 위한 props 추가 year, month 옵셔널로 변 renderCalendarItems 스타일 추가 DayItemWrapper에 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.
안녕하세요 노아 ㅎㅎ 엄청난 달력 로직 개선이 있었네요 ㄷㄷ
코드 다 읽어봤고, 특별히 기능상에서 문제 될만한 부분은 보이지 않았아요. 네이밍도 딱히?? 이해 안가는 것도 없었구요👍
노아가 재사용성에 초점을 맞춘 만큼, NPM README도 보면서 이 라이브러리 사용자 입장에서 캘린더 컴포넌트를 보고 느낀 점을 공유 해드리겠습니다 ㅎㅎ
-
Calendar.Item의 사용이 살짝 불편한 것 같아요.
현재 Calendar의 Item은 외부에서 map을 돌린 뒤 반환되는 Item을 children으로 넣어주는 방식인 것 같은데, 개인적으론 외부에서 매번 map을 돌리고 children으로 넘기는 방식이 직관적이지 않다?고 느껴졌어요. map 사용을 사용자에게 위임하기보단, 사용자는 배열이나 객체 형태로 넘기고, 반복문은 컴포넌트 내부에서 도는건 어떨까요? -
컴포넌트가 상당히 부모 의존적이에요.
사용자에게 자율성을 주기 위한 의도는 잘 알겠지만, 컴포넌트에 프롭스를 넘겨주는 방식이 꽤 부모 의존적인 것 같아요.(부모에서 state와 setState를 필수적으로 선언해야한다는 점 등) 지금도 좋은 컴포넌트지만, 이런 부분이 독립적으로 작동할 수 있게되면 더 좋은 컴포넌트가 될 것 같아요. 하지만 이 부분을 해결하기 위해선 많은 고민이 필요할 것 같아요. 저도 딱히 더 좋은 로직이 떠오르진 않네요..
두 가지 피드백 모두 빠르게 반영하기는 힘들고, 고민이 많이 필요할 것이라 생각합니다. 그래서 일단은 어프루브하고 나중에 천천히 생각해보면서 점차 개선해나가면 좋을 것 같아요 고생하셨습니다 ㅎ
* | ||
* * @default false | ||
*/ | ||
dataLoading?: boolean; |
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.
isDataLoading
는 어떤가요?
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.
is
prefix 좋은거 같아요!
룩소! 리뷰 감사해요! 1. Calendar.Item의 사용이 살짝 불편한 것 같아요.
2. 컴포넌트가 상당히 부모 의존적이에요.달력은 특정 속성이 존재하는 컴포넌트라고 생각해요. 여기에서 특정 속성은 년, 월 혹은 시작일, 마지막일이 됩니다. 이러한 속성을 외부(즉, 부모컴포넌트)에 전달하기 위한 여러 함수가 있어요. 이를 바탕으로 외부와 소통을 하게 됩니다. 보다 일관된 형식으로 구성을 한다면 훅을 제공하는 방법이 떠오릅니다. 그러기 위해서 Provider도 적용을 해야 하는데, 오히려 이렇게 된다면 초기 설정이 더 많아지지 않나? 라는 생각도 듭니다! |
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.
많은 개선이 있었네요!
주석을 잘 달아주셔서 변경 핵심안을 이해할 수 있었어요.
고생 많으셨습니다. Approve 할게요~
관련 이슈
closed #654
구현 기능 및 변경 사항
공유내용
1. 달력의 상태를 외부와 공유한다.
Calendar 컴포넌트의 경우
year
,month
또는date
을 DatePicker 컴포넌트의 경우startDate
,endDate
을 외부인 부모 컴포넌트에 전달하기 위한 메서드가 있습니다.예를 들어 Calendar 컴포넌트의 props 중에
onChangeCalendar
는 메서드로year
과month
를 매개변수로 받습니다. 즉, 부모 컴포넌트에서onChangeCalendar
을 전달할 때 자동으로Calendar
의year
과month
를 받을 수 있습니다. 이벤트 핸들러가 콜백함수에서 이벤트를 객체를 매개변수로 받을 수 있는것과 같아요!위와 같이 사용하여 외부에서 특정 년, 월에 해당하는 데이터를 불러올 수 있습니다.
DatePicker도 마찬가지로 DatePicker에서 선택된
startDate
,endDate
을onChangeDate
메서드를 통해 부모 컴포넌트로 전달하여 후속 작업을 진행할 수 있습니다.2. Calendar 컴포넌트에서 UI와 Data 분리
기존에는 Calendar 컴포넌트에서 UI와 Data fetching를 모두 담당했어요. 이를 이번 리팩터링을 통해 분리했습니다. 분리한 이유는 다음과 같아요.
이와 같은 이유로 데이터를 분리를 시도했지만 어떻게 하면 좋을지 많이 고민했습니다. 고민하는 과정에서 합성 컴포넌트를 생각하게 되었고 이를 적용했습니다. 아래는 실제 코드 입니다.
Calendar 컴포넌트는
Calendar.Item
를 자식 컴포넌트로 받을 수 있습니다. 주의해야 할 점은Calendar.Item
에date
속성을 반드시 넘겨줘야 하는데 이를 통해 어디에 해당Calendar.Item
가 위치해야 하는지 알 수 있기 때문입니다.결론은,
calendarData
라는 data fecthing을 통해 받아온 외부 데이터를 합성 컴포넌트로 전달하는 것입니다. 이때 외부 데이터엔 Date 타입을 가지는 값이 하나 있어야 하죠!3. 다양한 쓰임세의 DatePicker
현재 하루스터디에서는 startDate, endDate가 모두 필요한 DatePicker가 필요합니다. 하지만 특정 하루만 필요한 경우도 있기 때문에 이를 고려해서 DatePicker 컴포넌트를 만들었습니다. 이런 상황에 대한 스토리는 스토리북에 추가하였으니 확인할 수 있습니다!
공통 컴포넌트는 복잡하더라도 사용하는 측면에서는 최대한 간편하게 사용할 수 있도록 고민해봤어요. 보시고 개선되었으면 하는 것이 있다면 리뷰 부탁드려요!(특히 네이밍.. 😭)