fix: solve side effects related to duplicate events #1226

heenakwag · 2022-07-25T12:14:36Z

Please check if the PR fulfills these requirements

It's the right issue type on the title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has a description of the breaking change

Description

Fix an issue: the layout is broken when the collide event differs for each duplicate event.

Before	After

Fix an issue: going and coming durations are not showing in the collapsed event.

Before	After

Fix an issue: when there are too many events in one column, the width of the expanded event is less than the collapsed one.

Before	After

The width of the collapsed event was `5%` of the column.	Now, the width of the collapsed event is `9px`.

Add an example for collapseDuplicateEvent option.
Update docs for collapseDuplicateEvent option.

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

…oords of time event

…tDuplicateEvents option

heenakwag · 2022-07-25T12:16:09Z

apps/calendar/examples/00-calendar-app.html

@@ -107,6 +107,10 @@
              />
            </button>
            <span class="navbar--range"></span>
+            <div class="nav-checkbox">
+              <input class="checkbox-collapse" type="checkbox" id="collapse" value="collapse" />
+              <label for="collapse">Collapse duplicate events and disable the detail popup</label>


중복 일정이 겹쳐지고 펼쳐지는 동작이 잘 보이도록 collapseDuplicateEvents를 활성화하면 useDetailPopup를 끄도록 만들었습니다.

heenakwag · 2022-07-25T12:21:21Z

apps/calendar/src/controller/column.ts

@@ -266,11 +314,11 @@ export function setRenderInfoOfUIModels(

  matrices.forEach((matrix) => {
    const maxRowLength = Math.max(...matrix.map((row) => row.length));
-    const baseWidth = 100 / maxRowLength;
+    const baseWidth = Math.round(100 / maxRowLength);


중복 일정의 경우 preact에 스타일을 지정할 때 left와 width를 문자열값으로 넘기게 됩니다. 이 때 나누어 떨어지지 않는 baseWidth를 가지는 경우 css 스타일이 제대로 적용되지 못하는 경우가 생겨서 반올림하도록 하였습니다.

~~#### 기존에 문제가 발생하지 않았던 이유~~

기존에는 left와 width를 number 형식으로 preact에 넘기고, preact 내부에서 소숫점을 처리해주었습니다. 하지만 현재 중복된 일정의 경우 left와 width를 string 형식으로 넘기도록 변경되면서 preact에서 자동으로 처리되지 않습니다.

그냥 궁금해서요. preact 내부에서 소숫점 처리해줬다는 부분이 여긴가요?

https://github.com/preactjs/preact/blob/17da4efa736f14c84cd9f36fca4420d94f0dd403/src/diff/props.js#L36-L46

소숫점이 너무 많아지는 부분은 예전에도 뭔가 반올림 처리를 하거나 toFixed 를 써야 하지 않을까 생각했다가 괜찮으니까 냅뒀는데, 이런부분에서 의외의 문제가 터지는군요 🤔

아 이 부분은 제가 잘못 파악했네요. 기존에도 number가 아닌 string으로 넘기고 있었습니다. (참고 - TimeEvent.tsx L75-L79)

제 생각에는 이번에 추가된 퍼센트 값을 더하고 빼는 과정 때문에 부동소수점 오류가 난 것 같습니다.
기존에는 부동소수점의 연산이 아니라 100 / baseWidth 와 같이 결과가 부동소수점으로 나타나는 거라 문제가 없었는데, 이번에 추가된 로직은 부동소수점끼리의 연산이라 문제가 발생한 것 같아요.

heenakwag · 2022-07-25T12:24:03Z

apps/calendar/src/model/eventUIModel.ts

+  /**
+   * represent the left value of a duplicate event.
+   * ex) calc(50% - 24px), calc(50%), ...
+   *
+   * @type {string}
+   */
+  duplicateLeft = '';
+
+  /**
+   * represent the width value of a duplicate event.
+   * ex) calc(50% - 24px), 9px, ...
+   *
+   * @type {string}
+   */
+  duplicateWidth = '';


기존의 left와 width를 수정하지 않고 새로운 프로퍼티를 만든 이유는 타입 문제 때문입니다. left와 width가 문자열도 가능하도록 number | string 타입으로 바꾸면 다른 많은 곳에서 타입 에러가 발생하고, 이를 모두 수정해야 합니다. 생각보다 영향을 끼치는 부분이 많아서 새로운 프로퍼티를 만들었습니다.

heenakwag · 2022-07-25T12:26:56Z

apps/calendar/src/model/eventUIModel.ts

+        infos.push({
+          start: event.duplicateStarts as TZDate,
+          end: event.duplicateEnds as TZDate,
+          goingDuration: 0,
+          comingDuration: 0,
+        });


duplicateStarts에는 중복 일정 그룹 중 가장 이른 start + goingDuration을, duplicateEnds에는 가장 늦은 end + comingDuration을 저장합니다. duplicateStarts과 duplicateEnds는 이미 duration을 고려한 값이기 때문에 충돌을 계산할 때는 goingDuration과 comingDuration을 0으로 설정합니다.

adhrinae

리뷰 완료합니다. 어려운 기능 구현하느라 무척 고생하셨어요.

자잘하게 코드 다듬어주셨으면 하는 부분, 설명이 조금 더 필요하다고 생각하는 부분 등 코멘트 남겼습니다.

apps/calendar/playwright/week/timeGridEventClick.e2e.ts

apps/calendar/src/components/events/timeEvent.tsx

apps/calendar/src/factory/calendarCore.tsx

adhrinae · 2022-07-26T02:10:47Z

apps/calendar/src/controller/column.ts

@@ -266,11 +314,11 @@ export function setRenderInfoOfUIModels(

  matrices.forEach((matrix) => {
    const maxRowLength = Math.max(...matrix.map((row) => row.length));
-    const baseWidth = 100 / maxRowLength;
+    const baseWidth = Math.round(100 / maxRowLength);


그냥 궁금해서요. preact 내부에서 소숫점 처리해줬다는 부분이 여긴가요?

https://github.com/preactjs/preact/blob/17da4efa736f14c84cd9f36fca4420d94f0dd403/src/diff/props.js#L36-L46

소숫점이 너무 많아지는 부분은 예전에도 뭔가 반올림 처리를 하거나 toFixed 를 써야 하지 않을까 생각했다가 괜찮으니까 냅뒀는데, 이런부분에서 의외의 문제가 터지는군요 🤔

apps/calendar/src/controller/column.ts

apps/calendar/src/helpers/css.ts

lja1018

리뷰 완료합니다.

apps/calendar/src/controller/column.spec.ts

apps/calendar/src/controller/column.ts

lja1018 · 2022-07-26T08:06:16Z

apps/calendar/src/controller/column.ts

+  if (!isDuplicateEvent && uiModel.duplicateEvents.length > 0) {
+    uiModel.duplicateEvents.forEach((event) => {
+      setRenderInfo({
+        uiModel: event,
+        columnIndex,
+        baseWidth,
+        startColumnTime,
+        endColumnTime,
+        isDuplicateEvent: true,
+      });
+    });
+
+    return;
+  }


얼리리턴보다

Suggested change

if (!isDuplicateEvent && uiModel.duplicateEvents.length > 0) {

uiModel.duplicateEvents.forEach((event) => {

setRenderInfo({

uiModel: event,

columnIndex,

baseWidth,

startColumnTime,

endColumnTime,

isDuplicateEvent: true,

});

});

return;

}

if (isDuplicateEvent || uiModel.duplicateEvents.length === 0) {

// 아래 로직

} else {

uiModel.duplicateEvents.forEach((event) => {

setRenderInfo({

uiModel: event,

columnIndex,

baseWidth,

startColumnTime,

endColumnTime,

isDuplicateEvent: true,

});

});

}

같은 방식은 어떠신가요?

저는 개인적으로 얼리 리턴을 더 선호합니다..ㅎㅎㅎ 굳이 else 문을 만들어서 인덴테이션 한 단계를 추가하고 싶지 않기도 하고, 아래 로직에서도 return하는 로직이 없어서 어느 로직을 타더라도 리턴 타입이 동일하게 undefined이 되거든요.

혹시 else 쓰는 걸 선호하는 이유가 있나요??

지금 함수는 괜찮은데 함수가 길어지고 로직이 복잡할 때 중간에 얼리리턴으로 빠져나가면 로직 파악이 쉽지 않더라구요.

개인적인 의견이라 반영 안하셔도 괜찮습니다!

저도 기본적으론 얼리리턴 좋아하는데 아무것도 리턴 안하고 return 만 있는 얼리리턴보단 값이 명시적인 얼리리턴이어야 그럴만한 가치가 있다고 생각하는 편이에요.

이 함수는 약간 다른데, 재귀함수이므로 조건 종료 부분이 먼저 함수 상단에 드러나있는게 좋다고 생각해서 지금 상태로도 충분하다 봅니다.

제 코멘트에도 썼지만 이런 형태의 재귀가 아닌 다른 방식으로 풀어나갈 수 있으면 좋겠지만 마땅한 개선 아이디어가 떠오르지 않아 넘어가시죠.

heenakwag added 10 commits July 25, 2022 21:00

fix: consider coords of other duplicate events when calculating the c…

de6241a

…oords of time event

test: check duplicate events are sorted according to the result of ge…

10f4231

…tDuplicateEvents option

fix: set inner heights of collapsed duplicate events

27866f8

fix: set the width of collapsed event to 9px

bb8ee16

fix: round off baseWidth

0849fbb

docs: add a checkbox for collapseDuplicateEvents in the example

95404a6

docs: add jsdoc for collapseDuplicateEvents option

fd861f7

docs: change the links in jsdoc to the English version

c5b61d5

docs: update screenshots

f169ec9

refactor: fix typo

745508a

heenakwag requested review from adhrinae and lja1018 July 25, 2022 12:15

heenakwag commented Jul 25, 2022

View reviewed changes

adhrinae suggested changes Jul 26, 2022

View reviewed changes

lja1018 reviewed Jul 26, 2022

View reviewed changes

refactor: apply code review (ref #1226)

1984bd5

adhrinae approved these changes Jul 27, 2022

View reviewed changes

Base automatically changed from feat/duplicate-events to main July 27, 2022 04:44

heenakwag merged commit 74a9807 into main Jul 27, 2022

heenakwag deleted the fix/duplicate-event-layout branch July 27, 2022 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: solve side effects related to duplicate events #1226

fix: solve side effects related to duplicate events #1226

heenakwag commented Jul 25, 2022

heenakwag Jul 25, 2022

heenakwag Jul 25, 2022 •

edited

Loading

adhrinae Jul 26, 2022

heenakwag Jul 26, 2022

heenakwag Jul 25, 2022

heenakwag Jul 25, 2022

adhrinae left a comment

adhrinae Jul 26, 2022

lja1018 left a comment

lja1018 Jul 26, 2022

heenakwag Jul 27, 2022

heenakwag Jul 27, 2022

lja1018 Jul 27, 2022

adhrinae Jul 27, 2022

fix: solve side effects related to duplicate events #1226

fix: solve side effects related to duplicate events #1226

Conversation

heenakwag commented Jul 25, 2022

Please check if the PR fulfills these requirements

Description

Choose a reason for hiding this comment

heenakwag Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adhrinae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lja1018 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heenakwag Jul 25, 2022 •

edited

Loading