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] 스케줄 바를 드래그하여 수정할 수 있는 기능 구현 #857

Merged
merged 54 commits into from
Dec 11, 2023

Conversation

wzrabbit
Copy link
Collaborator

@wzrabbit wzrabbit commented Nov 13, 2023

[FE] 스케줄 바를 드래그하여 수정할 수 있는 기능 구현

이슈번호

close #846

PR 내용

본 PR에서는 캘린더의 스케줄 바를 드래그하여 일정을 수정할 수 있는 기능을 구현하였다.

  • 스케줄 바를 드래그하여 움직일 때 드래그 중인 스케줄 바만이 강조되어 표시된다.
  • 드래그 도중 위치에 따라 스케줄 바의 모양이 계속해서 변하도록 구현하였다.
  • 스케줄 바의 드래그 위치에 따라 어느 위치에 일정이 놓일지가 회색 윤곽선으로 표시된다.

원리 및 부연 설명

본 기능에 관여하는 컴포넌트 및 커스텀 훅은 아래와 같다.

  1. useScheduleBarDragStatus - 지금 스케줄 바가 드래그 중인 상태인지, 드래그 중인 스케줄 바는 무엇인지 등 드래그 관련 정보를 관리하고, 드래그 상태가 변경되었을 때 비동기 요청을 보내기도 하는 커스텀 훅. TeamCalendar 내부에서 사용한다.
  2. CalendarDragScreen - 스케줄 바를 옮기기 시작하면 화면에 보이며, 화면에 보이는 동안은 사용자의 마우스 포인터가 움직임에 따라 스케줄 바의 위치와 모양을 변경시키면서 마치 드래그 중인 것처럼 시각적인 요소를 제공하는 컴포넌트. 마우스를 놓아 드래그가 끝날 경우 업데이트해야 하는 스케줄의 정보를 전달한다.
  3. FakeScheduleBarsScreen - CalendarDragScreen 의 하위 컴포넌트. 스케줄 바 하나를 props로 받으면 가짜 스케줄 바 또는 스케줄 바가 놓이는 위치를 시각적으로 표현한다. 더 작은 단위의 시각적인 요소를 제공한다고 보면 무방하다.
  4. ScheduleBar - 이미 구현되어 있던 컴포넌트지만, 이제는 드래그를 감지할 수 있도록 개선되었다.
  5. useCalendarDragScreen - 드래그 중인 스케줄의 정보와 마우스 좌표를 기반으로 상대좌표를 계산하여 화면에 표시될 스케줄 바의 위치를 정하고, 일정 변경이 반영된 스케줄 바를 반환하여 스케줄 바의 모양을 정할 수 있는 커스텀 훅. CalendarDragScreen 내부에서 사용한다.
  6. generateScheduleBarsByMousePoint - 스케줄 바를 생성하는 유틸 함수와 비슷하지만, 마우스 좌표와 캘린더 크기 등의 부가 정보를 기반으로 변경된 모양의 스케줄 바를 시작/끝 날짜와 함께 반환하는 유틸 함수이다.

전반적인 플로우는 아래와 같다.
image

그리고, 그 중 CalendarDragScreen 컴포넌트에 대해 좀 더 자세히 설명하자면 아래와 같다.
image

의논할 거리

  1. 마우스를 놓으면 일정 수정 모달에서 확인 버튼을 누른 것과 동일한 로직을 사용하기는 하는데, 기존의 함수를 어떻게 재활용해야 할 지 잘 모르겠어서 핵심 코드만 복사해와서 새로 선언하는 방법을 사용했어... 개선이 필요해 보이는데 좋은 방법 있으면 부디 제안해 주면 좋을듯!
  2. debouncethrottle 필요하려나?

참고 자료

3.mp4

- 스케줄 바 드래그 현황에 대한 데이터를 가짜 드래그 스크린에 prop으로 넘길 수 있도록 값 관리
- 스케줄 바의 드래그를 시작하는 경우 / 드래그를 중지하는 경우에 따라 값을 업데이트할 수 있도록 관리
- 이후 마우스 좌표를 받아 마우스를 따라가는 위치에 렌더링되도록 사용 예정
- display: none을 이용하여 커스텀 훅의 state와 listener를 유지
- '칸 이동 수치'란 시작 좌표를 기준으로 행, 열을 얼마나 이동했는지를 의미하는 수치이다. 이는 캘린더 바의 모양을 어떻게 정할지를 계산하는 데 사용된다.
- onMouseDown 이벤트로 인해 스케줄 바에 해당하는 일정 모달이 띄워지지 않는 문제가 있음
- 스케줄 바가 이제 여러 목적(일반, 장식용, 인디케이터)으로 쓰이므로, mode를 통해 목적을 정한다.
- no-interaction: 스케줄 바에 hover을 해도 반응이 없으며, 포인터가 바뀌지 않는 등 상호작용이 불가능해진다. 시각적인 용도로 사용해야 할 때 사용한다.
- indicator 모드: 상호작용이 불가능하며, 스케줄 바의 윤곽만 나타난다. 스케줄 바가 놓일 위치를 시각적으로 표시하는 데에 사용한다.
- 스케줄 바를 인디케이터 형태로 보여주는 컴포넌트
- 스케줄 바에서 정보를 가져오는 데 이동된 스케줄 바가 더 이상 렌더링되지 않으므로 생기는 문제. 스케줄 바가 비어 있더라도 일정 시작과 끝 정보는 보존되도록 유틸 함수를 수정
- 꼭 필요한 경우에만 생성되도록 한쪽에 쏠린 dependency array를 배분
- 이전 캘린더 바의 이동에서 남아있는 상대좌표의 값이 이후의 캘린더 바의 이동에 영향을 끼친 것. 이동 완료 후 상대좌표의 값을 0으로 초기화시키는 것으로 해결
- 직접 해당 타입을 스케줄 바에서 사용하게 되는 일이 많아짐에 따라 추가하게 됨
@wzrabbit wzrabbit force-pushed the feat/fe/캘린더-바-드래그 branch from 4f9c241 to 4981756 Compare November 14, 2023 05:53
@wzrabbit wzrabbit marked this pull request as ready for review November 14, 2023 06:46
Copy link
Collaborator

@hafnium1923 hafnium1923 left a comment

Choose a reason for hiding this comment

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

오우 진짜 퀄리티 높은 코드네 보면서 감탄했어!
작성하느라 진짜진짜 고생 많았음!!! 리뷰 기다려줘서 고마워ㅎㅎ 아까 자세하게 설명해준게 도움이 컸어
리뷰는 간단한거 몇개만 작성했는데 한번 확인해주면 좋을 것 같아~!!
수고많았어 토끼야

schedule: Schedule;
onMouseUp: (
title: string,
startDateTime: Schedule['startDateTime'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

image
일케 안한 이유가 있어??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아니, 단순히 YYYYMMDDHHMM 이 타입을 잊고 있었어...
이게 의미도 더 명확하게 전달되는 것 같아 보이니, 이 타입을 사용하도록 코드 수정할게!

@@ -224,14 +229,21 @@ const TeamCalendar = (props: TeamCalendarProps) => {
return <S.DayOfWeek key={day}>{day}</S.DayOfWeek>;
})}
</S.DaysOfWeek>
<div>
<S.CalendarGrid>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 그리드라고 하니까 진짜 그리드같아서 네이밍 바꾸면 좋을 것 같애~!@

} as const;

export const useScheduleDragStatus = () => {
const [dragStatus, setDragStatus] = useState<DragStatus>({
Copy link
Collaborator

Choose a reason for hiding this comment

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

�컨벤션 확인해봤는데 커스텀 타입의 경우에는 타입과 초기값을 명시하는게 맞네! 다만 나는 schedule에 emptySchdule을 사용하는 것 보다 단언을 사용하는게 좀 더 좋을 것 같아. 만약 빈 값이라 에러가 나오게 되는 경우 때문이라면 이 경우는 아무것도 선택되지 않았을 때 어떤 동작을 하려고 하는 것이니 오히려 더 빡빡하게 에러처리를 해두는게 낫다고 보거든

Copy link
Collaborator Author

@wzrabbit wzrabbit Dec 7, 2023

Choose a reason for hiding this comment

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

일단, TypeScript에서의 단언 사용은 최대한 피하려 하는 편이야. 단언을 사용해야 한다는 것 자체가 개발자의 의도(=타입)에 반하는 값이 들어갈 가능성이 있다는 의미인데, 이게 당장은 잘못된 값이 들어갈 일이 발생할 확률이 없더라도 이후에 코드의 로직을 수정하면서 잘못된 값이 들어갈 때에 대한 대비가 안 되어 있어 터지는 경우가 생길 수 있을 것 같아.

그렇다고 지금과 같이 emptySchedule 을 따로 정의해놓는 방법도 좋은 방법은 전혀 아닌 것 같은 게, 이후 저 emptySchedule 이라는 데이터가 나중에 실수로라도 사용될 경우 역시 오류가 발생할 가능성이 있어 보이더라고(id 가 애초에 0이야). 리뷰하는 입장에서 더러워보이는 것도 있고...

그래서 고민해 본 결과, 타입에서 schedule 프로퍼티의 값을 null도 허용하는 방향으로 바꾸어보았어. 드래그 중이 아닐 때에는 스케줄의 정보가 없을테니, emptySchedule이라는 값으로 땜빵하는 것보다 차라리 null 이라는 값을 사용해 의도를 드러내는 게 좋다는 생각이 들었어.

아무것도 선택되지 않았을 때 어떤 동작을 하려고 하는 것이니

이제 schedule 프로퍼티에서 null 값이 허용되잖아? 아무것도 선택되지 않았을 때 어떤 동작을 할 일이 없도록 직접 조건문을 달아서 처리해 주었어. emptySchedule 값을 이용해 땜빵하는 것보다 안전한 접근이 된 것 같다고 생각해 (당연히 타입 추론도 되고)

const handleMouseUp = useCallback(() => {
  if (!isDragging || !scheduleBarsInfo || !schedule) {
    return;
  }
 
 // 서버와 소통하는 커스텀 훅으로 데이터를 보내는 로직이 오는 자리
}

결론적으로 아예 타입에 null 을 넣음으로써 "스케줄은 상황에 따라 값이 null일수도 있으니 null인 경우를 대비하세요" 라는 의도를 전달하고 대비책도 조건문(-> 타입가드)으로 해결하는 방법을 사용한 거야! 이 방법은 어떤 것 같아?

Copy link
Collaborator

Choose a reason for hiding this comment

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

아주좋아

},
{
onSuccess: () => {
showToast('success', '일정이 수정되었습니다.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

mutate가 성공한다면 여기에 dragStatus를 초기화 하는 로직이 있으면 좋을 듯

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞아, 이게 더 의도도 드러나고 내가 이야기했던 버그도 발생하지 않도록 할 수 있는 좋은 방법 같다, 꼭 적용해 둘게!

startDateTime,
endDateTime,
},
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

추가적으로 제안하자면 스케줄 수정의 경우는 onMutate를 적용해 낙관적 업데이트를 하면 좋을 것 같애. 드래그로 수정하는 것 자체가 사용자에게 바로 피드백을 주고 싶기 때문에 기능을 추가했다고 생각하거든. 그렇다면 통신이 오래걸릴 때라도 바로 피드백을 줄 수있도록 하는게 어떨까? 다만 업데이트를 실패했을 때 사용자 경험에 치명적이라고 생각한다면 낙관적업데이트가 필요없을 것 같기두 하고!
만약 너도 낙관적 업데이트에 동의한다면 이 PR이 끝난 후 스케줄 수정에 낙관적업데이트를 적용하는 건 요토가 해보는게 어떨까 싶어(요토가 예전부터 tanstack-query 제대로 해본적 없다고 아쉬워했던게 기억나서)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

전적으로 루루의 의견에 동의하고, 나도 필요성을 느껴

  • 성공률: 일정 수정이라면 성공률은 충분히 높을 것 같아, 사용자가 직접 모달을 띄워서 내용을 수정하는 것이 아닌 정해진 범위로만 드래그를 해서 바꾸는 형식이기 때문에 더더욱 높을 것 같아
  • UX: 드래그를 완료했는데 잠시 드래그 전으로 스케줄 바가 이동했다가 반영 이후 드래그 후로 스케줄 바가 다시 이동하는 현상은 충분히 사용자에게 어색함을 줄 수 있는 요소로 보여

낙관적 업데이트는 다른 PR에서 작업할 수 있도록 해 볼게

@@ -17,6 +17,10 @@ export const CalendarHeader = styled.div`
justify-content: space-between;
`;

export const CalendarGrid = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grid 로끝나니까 좀 어색한 것 같아..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

전체적으로 캘린더를 2차원 격자로 관리하고 있어서 이러한 네이밍을 사용했는데, 혹시 더 괜찮은 이름이 생각난다면 추천해 줄 수 있을까?

나도 좋은 이름을 짓고 싶었지만, TeamCalendar 컴포넌트의 규모가 꽤 커서 겹치는 이름들이 많았던지라, 이름을 짓기가 어려웠던 것 같아...

Copy link
Collaborator

Choose a reason for hiding this comment

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

그러니까 말야 그냥 일단일케 둘까? 하도 캘린더가 구조가 복잡한데 단일형태라그런지 마땅하지 않네

import type { CalendarSize } from '~/types/size';
import { useCalendarDragScreen } from '~/hooks/schedule/useCalendarDragScreen';

interface CalendarDragScreenProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

...dragStaus로 보내고 있으면 dragStatus 객체로 받는게 더 깔끔하지 않나?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

나도 그렇게 생각하고, 반영해 둘게!

import type { CalendarSize } from '~/types/size';

interface UseCalendarDragScreenProps {
visible: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

\visible보다는 dragStats의 isDragging을 그대로 쓰는게 좀 더 명확할 것 같애

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

위 코멘트에서 dragStatus 로 받도록 변경했으니 드래그 관련 정보는 객체로 받을 것 같고, 자연스럽게 visibledragStatus 로 변경될거야~

@hafnium1923 hafnium1923 removed the request for review from suyoungj November 30, 2023 07:18
Copy link
Collaborator Author

@wzrabbit wzrabbit left a comment

Choose a reason for hiding this comment

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

리뷰 고맙고~
엄청 길고 못생긴 코드라 대충 넘어갈 수도 있었는데 관심 가지고 이 코드가 어떻게 작동하는건지 궁금한 점을 여과없이 물어봐 줘서 좋았어~

여러 제안을 해 주었는데 대체로 좋은 제안이라 생각되서 대부분 수용했고, 단언에 대해서는 단언을 사용하는 대신 다른 해결 방법을 찾아보았어
CalendarGrid 이름은 좋은 이름 좀 추천해주라 나도 모르겠다...

그럼 코멘트 확인해 줘! 고마워~

schedule: Schedule;
onMouseUp: (
title: string,
startDateTime: Schedule['startDateTime'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아니, 단순히 YYYYMMDDHHMM 이 타입을 잊고 있었어...
이게 의미도 더 명확하게 전달되는 것 같아 보이니, 이 타입을 사용하도록 코드 수정할게!

},
{
onSuccess: () => {
showToast('success', '일정이 수정되었습니다.');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞아, 이게 더 의도도 드러나고 내가 이야기했던 버그도 발생하지 않도록 할 수 있는 좋은 방법 같다, 꼭 적용해 둘게!

startDateTime,
endDateTime,
},
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

전적으로 루루의 의견에 동의하고, 나도 필요성을 느껴

  • 성공률: 일정 수정이라면 성공률은 충분히 높을 것 같아, 사용자가 직접 모달을 띄워서 내용을 수정하는 것이 아닌 정해진 범위로만 드래그를 해서 바꾸는 형식이기 때문에 더더욱 높을 것 같아
  • UX: 드래그를 완료했는데 잠시 드래그 전으로 스케줄 바가 이동했다가 반영 이후 드래그 후로 스케줄 바가 다시 이동하는 현상은 충분히 사용자에게 어색함을 줄 수 있는 요소로 보여

낙관적 업데이트는 다른 PR에서 작업할 수 있도록 해 볼게

import type { CalendarSize } from '~/types/size';
import { useCalendarDragScreen } from '~/hooks/schedule/useCalendarDragScreen';

interface CalendarDragScreenProps {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

나도 그렇게 생각하고, 반영해 둘게!

import type { CalendarSize } from '~/types/size';

interface UseCalendarDragScreenProps {
visible: boolean;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

위 코멘트에서 dragStatus 로 받도록 변경했으니 드래그 관련 정보는 객체로 받을 것 같고, 자연스럽게 visibledragStatus 로 변경될거야~

@@ -17,6 +17,10 @@ export const CalendarHeader = styled.div`
justify-content: space-between;
`;

export const CalendarGrid = styled.div`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

전체적으로 캘린더를 2차원 격자로 관리하고 있어서 이러한 네이밍을 사용했는데, 혹시 더 괜찮은 이름이 생각난다면 추천해 줄 수 있을까?

나도 좋은 이름을 짓고 싶었지만, TeamCalendar 컴포넌트의 규모가 꽤 커서 겹치는 이름들이 많았던지라, 이름을 짓기가 어려웠던 것 같아...

} as const;

export const useScheduleDragStatus = () => {
const [dragStatus, setDragStatus] = useState<DragStatus>({
Copy link
Collaborator Author

@wzrabbit wzrabbit Dec 7, 2023

Choose a reason for hiding this comment

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

일단, TypeScript에서의 단언 사용은 최대한 피하려 하는 편이야. 단언을 사용해야 한다는 것 자체가 개발자의 의도(=타입)에 반하는 값이 들어갈 가능성이 있다는 의미인데, 이게 당장은 잘못된 값이 들어갈 일이 발생할 확률이 없더라도 이후에 코드의 로직을 수정하면서 잘못된 값이 들어갈 때에 대한 대비가 안 되어 있어 터지는 경우가 생길 수 있을 것 같아.

그렇다고 지금과 같이 emptySchedule 을 따로 정의해놓는 방법도 좋은 방법은 전혀 아닌 것 같은 게, 이후 저 emptySchedule 이라는 데이터가 나중에 실수로라도 사용될 경우 역시 오류가 발생할 가능성이 있어 보이더라고(id 가 애초에 0이야). 리뷰하는 입장에서 더러워보이는 것도 있고...

그래서 고민해 본 결과, 타입에서 schedule 프로퍼티의 값을 null도 허용하는 방향으로 바꾸어보았어. 드래그 중이 아닐 때에는 스케줄의 정보가 없을테니, emptySchedule이라는 값으로 땜빵하는 것보다 차라리 null 이라는 값을 사용해 의도를 드러내는 게 좋다는 생각이 들었어.

아무것도 선택되지 않았을 때 어떤 동작을 하려고 하는 것이니

이제 schedule 프로퍼티에서 null 값이 허용되잖아? 아무것도 선택되지 않았을 때 어떤 동작을 할 일이 없도록 직접 조건문을 달아서 처리해 주었어. emptySchedule 값을 이용해 땜빵하는 것보다 안전한 접근이 된 것 같다고 생각해 (당연히 타입 추론도 되고)

const handleMouseUp = useCallback(() => {
  if (!isDragging || !scheduleBarsInfo || !schedule) {
    return;
  }
 
 // 서버와 소통하는 커스텀 훅으로 데이터를 보내는 로직이 오는 자리
}

결론적으로 아예 타입에 null 을 넣음으로써 "스케줄은 상황에 따라 값이 null일수도 있으니 null인 경우를 대비하세요" 라는 의도를 전달하고 대비책도 조건문(-> 타입가드)으로 해결하는 방법을 사용한 거야! 이 방법은 어떤 것 같아?

Copy link
Collaborator

@hafnium1923 hafnium1923 left a comment

Choose a reason for hiding this comment

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

길고도 커다란 업데이트가 드디어 적용되네 고생많았어 요토! 실제로 사용하는게 기대된다ㅎㅎ

@hafnium1923 hafnium1923 merged commit ea409b0 into develop Dec 11, 2023
1 check passed
@hafnium1923 hafnium1923 deleted the feat/fe/캘린더-바-드래그 branch December 11, 2023 04:19
@wzrabbit wzrabbit restored the feat/fe/캘린더-바-드래그 branch December 18, 2023 17:02
@wzrabbit wzrabbit deleted the feat/fe/캘린더-바-드래그 branch December 25, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FE] 일정 스케줄바를 드래그해서 수정하는 기능 구현
2 participants