Skip to content
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

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

nlom0218
Copy link
Collaborator

관련 이슈

closed #654

구현 기능 및 변경 사항

  • 달력 기반 공통 컴포넌트 구현
    • 일정, 투두리스트와 같은 달력은 Calendar 컴포넌트
    • 날짜를 선택할 수 있는 달력은 DatePicker 컴포넌트

공유내용

1. 달력의 상태를 외부와 공유한다.

Calendar 컴포넌트의 경우 year, month 또는 date을 DatePicker 컴포넌트의 경우 startDate, endDate을 외부인 부모 컴포넌트에 전달하기 위한 메서드가 있습니다.

예를 들어 Calendar 컴포넌트의 props 중에 onChangeCalendar는 메서드로 yearmonth를 매개변수로 받습니다. 즉, 부모 컴포넌트에서 onChangeCalendar을 전달할 때 자동으로 Calendaryearmonth를 받을 수 있습니다. 이벤트 핸들러가 콜백함수에서 이벤트를 객체를 매개변수로 받을 수 있는것과 같아요!

const Ex = () => {
  return  <Calendar onChangeCalendar={(year, month) => console.log(year, month}> // 달력의 year과 month를 받을 수 있음.
    // ...
  </Calendar>
}

위와 같이 사용하여 외부에서 특정 년, 월에 해당하는 데이터를 불러올 수 있습니다.

DatePicker도 마찬가지로 DatePicker에서 선택된 startDate, endDateonChangeDate 메서드를 통해 부모 컴포넌트로 전달하여 후속 작업을 진행할 수 있습니다.

2. Calendar 컴포넌트에서 UI와 Data 분리

기존에는 Calendar 컴포넌트에서 UI와 Data fetching를 모두 담당했어요. 이를 이번 리팩터링을 통해 분리했습니다. 분리한 이유는 다음과 같아요.

  1. Calendar 컴포넌트가 데이터에 의존하고 있다면 다른 곳에서 재사용하기 어렵다.(즉, 복붙을 해야 한다.)
  2. 달력이 스터디 기록 뿐 아니라 다른 곳에서 사용될 경우 일관적인 UI를 유지하고 싶다.

이와 같은 이유로 데이터를 분리를 시도했지만 어떻게 하면 좋을지 많이 고민했습니다. 고민하는 과정에서 합성 컴포넌트를 생각하게 되었고 이를 적용했습니다. 아래는 실제 코드 입니다.

const MemberRecordCalendar = ({ memberId }: Props) => {
 // ...

  return (
    <Calendar
      year={year}
      month={month}
      limit={3}
      dataLoading={isLoading}
      onClickDay={handleOpenMemberRecordListModal}
      onClickRestDataCount={handleOpenMemberRecordListModal}
      onClickTotalDataCount={handleOpenMemberRecordListModal}
      onChangeCalendar={(year, month) => updateYearMonth(year, month)}
    >
      {calendarData?.map((item) => (
        <Calendar.Item key={item.studyId} date={new Date(item.createdDate)}>
          <Record onClick={() => handleClickStudyItem(item.studyId)}>{item.name}</Record>
        </Calendar.Item>
      ))}
    </Calendar>
  );
};

Calendar 컴포넌트는 Calendar.Item를 자식 컴포넌트로 받을 수 있습니다. 주의해야 할 점은 Calendar.Itemdate 속성을 반드시 넘겨줘야 하는데 이를 통해 어디에 해당 Calendar.Item가 위치해야 하는지 알 수 있기 때문입니다.

결론은, calendarData라는 data fecthing을 통해 받아온 외부 데이터를 합성 컴포넌트로 전달하는 것입니다. 이때 외부 데이터엔 Date 타입을 가지는 값이 하나 있어야 하죠!

3. 다양한 쓰임세의 DatePicker

현재 하루스터디에서는 startDate, endDate가 모두 필요한 DatePicker가 필요합니다. 하지만 특정 하루만 필요한 경우도 있기 때문에 이를 고려해서 DatePicker 컴포넌트를 만들었습니다. 이런 상황에 대한 스토리는 스토리북에 추가하였으니 확인할 수 있습니다!


공통 컴포넌트는 복잡하더라도 사용하는 측면에서는 최대한 간편하게 사용할 수 있도록 고민해봤어요. 보시고 개선되었으면 하는 것이 있다면 리뷰 부탁드려요!(특히 네이밍.. 😭)

dataLoading을 위한 props 추가
year, month 옵셔널로 변
renderCalendarItems 스타일 추가
DayItemWrapper에 onClick 메서드 추가
@nlom0218 nlom0218 linked an issue Nov 10, 2023 that may be closed by this pull request
1 task
@nlom0218 nlom0218 self-assigned this Nov 10, 2023
Copy link
Collaborator

@woo-jk woo-jk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 노아 ㅎㅎ 엄청난 달력 로직 개선이 있었네요 ㄷㄷ
코드 다 읽어봤고, 특별히 기능상에서 문제 될만한 부분은 보이지 않았아요. 네이밍도 딱히?? 이해 안가는 것도 없었구요👍

노아가 재사용성에 초점을 맞춘 만큼, NPM README도 보면서 이 라이브러리 사용자 입장에서 캘린더 컴포넌트를 보고 느낀 점을 공유 해드리겠습니다 ㅎㅎ

  1. Calendar.Item의 사용이 살짝 불편한 것 같아요.
    현재 Calendar의 Item은 외부에서 map을 돌린 뒤 반환되는 Item을 children으로 넣어주는 방식인 것 같은데, 개인적으론 외부에서 매번 map을 돌리고 children으로 넘기는 방식이 직관적이지 않다?고 느껴졌어요. map 사용을 사용자에게 위임하기보단, 사용자는 배열이나 객체 형태로 넘기고, 반복문은 컴포넌트 내부에서 도는건 어떨까요?

  2. 컴포넌트가 상당히 부모 의존적이에요.
    사용자에게 자율성을 주기 위한 의도는 잘 알겠지만, 컴포넌트에 프롭스를 넘겨주는 방식이 꽤 부모 의존적인 것 같아요.(부모에서 state와 setState를 필수적으로 선언해야한다는 점 등) 지금도 좋은 컴포넌트지만, 이런 부분이 독립적으로 작동할 수 있게되면 더 좋은 컴포넌트가 될 것 같아요. 하지만 이 부분을 해결하기 위해선 많은 고민이 필요할 것 같아요. 저도 딱히 더 좋은 로직이 떠오르진 않네요..

두 가지 피드백 모두 빠르게 반영하기는 힘들고, 고민이 많이 필요할 것이라 생각합니다. 그래서 일단은 어프루브하고 나중에 천천히 생각해보면서 점차 개선해나가면 좋을 것 같아요 고생하셨습니다 ㅎ

*
* * @default false
*/
dataLoading?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isDataLoading는 어떤가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is prefix 좋은거 같아요!

@nlom0218
Copy link
Collaborator Author

nlom0218 commented Nov 19, 2023

룩소! 리뷰 감사해요!
리뷰 내용에 대해 답변 드리겠습니다 :)

1. Calendar.Item의 사용이 살짝 불편한 것 같아요.

Calendar.Item을 뺀 이유는 아이템들의 디자인, 이벤트를 컴포넌트에서 종속시키지 않고 외부에서 자유롭게 하기 위함이였어요. 때문에 item들을 클릭했을 때 어떤 이벤트가 발생하는지도 자유롭게 구성할 수 있습니다.
얼마만큼 자유도를 줘야 하는지 아니면 어느정도는 일관된 형식을 가지고 컴포넌트를 만들어야 하는지.. 이 부분이 항상 고민되네요. 결국엔 팀내에서 정하는 부분이겠지만요!

2. 컴포넌트가 상당히 부모 의존적이에요.

달력은 특정 속성이 존재하는 컴포넌트라고 생각해요. 여기에서 특정 속성은 년, 월 혹은 시작일, 마지막일이 됩니다. 이러한 속성을 외부(즉, 부모컴포넌트)에 전달하기 위한 여러 함수가 있어요. 이를 바탕으로 외부와 소통을 하게 됩니다. 보다 일관된 형식으로 구성을 한다면 훅을 제공하는 방법이 떠오릅니다. 그러기 위해서 Provider도 적용을 해야 하는데, 오히려 이렇게 된다면 초기 설정이 더 많아지지 않나? 라는 생각도 듭니다!

@nlom0218 nlom0218 requested a review from yeopto November 29, 2023 10:55
Copy link
Collaborator

@yeopto yeopto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

많은 개선이 있었네요!
주석을 잘 달아주셔서 변경 핵심안을 이해할 수 있었어요.
고생 많으셨습니다. Approve 할게요~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[FE] 달력 로직 개선
3 participants